-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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(ssr): externalize network imports during ssrLoadModule
#15599
Conversation
|
@hi-ogawa |
@hi-ogawa |
@hi-ogawa As we cannot use external urls that we do not have control over for testing, |
@hi-ogawa also verified your fix locally and it seems to solve the error, great work :) |
@omridevk Thanks for confirming the fix on your end! Regarding the tests, actually there was esm.sh dependency in other places in the past, but it looks like they was just removed recently due to test flakiness #14684 Thanks for the suggestion. For the purpose of the review, I think this is okay, but I'll think about avoiding external network request here. |
ssrLoadModule
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.
Great test! And this looks like the right spot to add the check.
I know there are Vite plugins that handles URL imports, but I think they should be resolving those import paths to a safe id / virtual id, so the nodeImport
never hits for those imports. So might be safe there.
Description
I haven't fully tested yet, but one cause of the issue is that ssr
nodeImport
is trying totryNodeResolve
on external url.I'll experiment this further and adding tests etc...(Now I added a test, but the way I did is probably overkill e.g.
esm.sh dependence,experimental flag, etc... This was a easy way to verify locally, so I started with this. I would appreciate any suggestions on how it should be tested.)Additional context
I made this repro https://github.com/hi-ogawa/repro-vite-ssr-network-imports to understand the current state. It looks like
transformRequest(... { ssr: true })
is preserving network imports properly butssrLoadModule
is failing because ofnodeImport/tryNodeResolve
.Network import is experimental on NodeJS, so users would need to enable
--experimental-network-imports
flags manually if they need this feature https://nodejs.org/api/esm.html#https-and-http-imports. (I wonder if there's any Vite-based SSR framework on Deno. if there is one, then I feel more people would be blocked by this.)I'm not sure about the actual usefulness of network import feature myself, but just letting the external url go through during ssr dev seems reasonable as it would also align with other places such as import analysis during build:
vite/packages/vite/src/node/plugins/importAnalysisBuild.ts
Lines 339 to 342 in 06de8f0
Additionally, there is an issue on Vitest vitest-dev/vitest#4949, but since vite-node doesn't use
ssrLoadModule
directly, I'm suspecting that the cause is different and Vite-side fix is not relevant for Vitest.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).