Skip to content

Conversation

@tophf
Copy link

@tophf tophf commented Nov 13, 2025

The problem I'm fixing here is that Node 25+ throws on reading localStorage that occurs implicitly in ...global:

    const vmContext = vm.createContext({
      ...global,

My fix is to use a "meta" clone via Object.getOwnPropertyDescriptors + Object.defineProperties, which copies literally every global and is arguably what f269fc0 intended to achieve. Hopefully there are no secret/sensitive globals used by this plugin.

@alexander-akait
Copy link
Collaborator

Sounds like a regression on Node.js side

@alexander-akait
Copy link
Collaborator

Reported - nodejs/node#60704 let's wait an answer

@slorber
Copy link

slorber commented Nov 13, 2025

This also prevents Docusaurus dev server from starting: facebook/docusaurus#11545

CleanShot 2025-11-13 at 17 05 37@2x

@slorber
Copy link

slorber commented Nov 13, 2025

I tried this patch locally for Docusaurus, and it didn't work under Node 25.2:

×   Error: require is not defined
  │ 
  │   - dev.html.template.ejs:63
  │     /projects/docusaurus/packages/docusaurus/lib/webpack/templates/dev.html.template.ejs:63:1
  │ 

I tried adding back the "require" removed, leading to another error:

×   Error: __webpack_require__ is not defined
  │ 
  │ 
  │   - eval
  │ 
  │   - dev.html.template.ejs:10 data:text/javascript,__webpack_public_path__ = __webpack_base_uri__ = htmlWebpackPluginPublicPath;
  │     /projects/docusaurus/packages/docusaurus/lib/webpack/templates/dev.html.template.ejs:10:1

I tried adding back everything you removed and keeping the spread:

...Object.defineProperties({}, Object.getOwnPropertyDescriptors(global))
×   Error: Value of "this" must be of type nullish or must be the global object
  │ 
  │   - exposed-window-or-worker:114 global.cryptoThisCheck
  │     node:internal/bootstrap/web/exposed-window-or-worker:114:15

No idea how to fix it.


Need to double check, but I also think it fails on Node 24:

×   TypeError: Cannot set property crypto of #<Object> which has only a getter
  │ 
  │   - Object.assign

Repro?:

git clone [email protected]:facebook/docusaurus.git
cd docusaurus
yarn install
yarn start:website

@alexander-akait
Copy link
Collaborator

@slorber Yeah, I think we will need to wait Node.js answer here, we need to pass globals here, technically we can filter as a workaround, but I still think it is a Node.js regression, let's wait an answer from them

@tophf
Copy link
Author

tophf commented Nov 13, 2025

This change in node is spec-compliant, so it's unlikely they'll revert it.

I've re-worked the fix to handle one problem that turned out to be caused by eval, however I couldn't ensure the test completes locally because eslint kept finding dozens of [objectively present] violations in the existing code that I didn't change and some of test:only suites claimed unexpected values while they looked exactly like the expected one.

The test:only has succeeded after I disabled git's autocrlf for the repo.

@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@6ef547a). Learn more about missing BASE report.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1880   +/-   ##
=======================================
  Coverage        ?   92.38%           
=======================================
  Files           ?       18           
  Lines           ?      749           
  Branches        ?      209           
=======================================
  Hits            ?      692           
  Misses          ?       54           
  Partials        ?        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alexander-akait
Copy link
Collaborator

@tophf I still think it is a bug and should be reverted, any way I am fine with these changes too, do can fix eslint problem?

@tophf
Copy link
Author

tophf commented Nov 14, 2025

fix eslint problem

In this PR? But that code is completely unrelated, so it'd be weird to change it here. What if I just exclude the file from eslint checks altogether and someone will deal with those issues separately?

@alexander-akait
Copy link
Collaborator

@tophf
Copy link
Author

tophf commented Nov 14, 2025

Yeah, I saw that. Thing is, it doesn't say what's wrong. If the project is using prettier I would expect it to be applied automatically without me doing anything. Should I run prettier explicitly myself?

@tophf
Copy link
Author

tophf commented Nov 14, 2025

Okay, I found and ran fix:prettier which fixed lint:prettier test, but I still can't commit the change due to lint:js which finds problems in this file that aren't mine, so I've just disabled the pre-commit hook and force-pushed the changes anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants