-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix(vite-node): externalize network imports #4987
fix(vite-node): externalize network imports #4987
Conversation
✅ Deploy Preview for fastidious-cascaron-4ded94 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…k-imports' into fix-vite-node-externalize-network-imports
// not supported? | ||
// FAIL test/basic.test.ts [ test/basic.test.ts ] | ||
// Error: ENOENT: no such file or directory, open 'http://localhost:9602/slash@3.0.0.js' | ||
// ❯ Object.openSync node:fs:596:3 | ||
// ❯ readFileSync node:fs:464:35 | ||
vmThreads: { | ||
execArgv: ['--experimental-network-imports'], | ||
}, |
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.
It looks like it's not working on vmThreads. I'll investigate further to see if it's simply NodeJS limitation.
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.
I think every import is delegated to our VM implementation, and it just doesn't support http
.
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.
Thanks for the hint! I haven't had a chance to look at how vmThreads works yet, so I'll look around a bit to get the idea.
I suppose it's still okay even if it doesn't work on vmThreads?
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.
Yes, we can disable this test for vm threads/forks for now.
I think it's a separate feature request (maybe create an issue for it). Shouldn't be hard to implement.
I am not sure how to contact you otherwise, but I sent an e-mail to the address listed in your GitHub profile. Please have a look when you have time 😄 |
Description
As shown in the reproduction, currently network import is not externalized by vite-node and Vite's
transformRequest
ends up with a following error:Vite's
ssrLoadModule
didn't support network imports previously but it is changed to externalize it in vitejs/vite#15599, so Vite-node side change also aligns with Vite.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Documentation
pnpm run docs
command.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.