-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(module-runner): don't return node builtins for getBuiltins unconditionally
#8863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
getBuiltins unconditionally
This reverts commit 83ebae5.
|
The test failure is caused by nodejs/node#60336 |
I doubt that it has any effect, because we do the builtin check ourselves:
The |
Hm, I guess for mocking it does matter that we go into |
|
I've updated the comment, does this make sense? |
|
I don't think it matters much for the server runner because it doesn't have a mocker and is just a light wrapper, but the comment makes sense in the runtime module runner 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Description
This PR fixes this ecosystem-ci failure: https://github.com/vitejs/vite-ecosystem-ci/actions/runs/18900807449/job/53947420616
It seems #8746 was wrong. It seems it was returning modules that should not be treated as builtin in some environments.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yamlunless you introduce a new test example.Tests
pnpm test:ci.Documentation
pnpm run docscommand.Changesets
feat:,fix:,perf:,docs:, orchore:.