-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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: optimize deps on dev SSR, builtin imports in node #8854
Conversation
✅ Deploy Preview for vite-docs-main ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Remove the new |
vite-ecosystem-ci run against this PR https://github.com/vitejs/vite-ecosystem-ci/actions/runs/2583218491 |
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 PR explanations. It made reviewing easier!
banner: | ||
platform === 'node' | ||
? { | ||
js: `import { createRequire } from 'module';const require = createRequire(import.meta.url);` | ||
} | ||
: undefined, |
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 we can put a comment above about why this is needed.
// Important! We use the non-ssr optimized deps to find known imports | ||
const depsOptimizer = getDepsOptimizer(server.config, { ssr: false }) |
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.
Perhaps we can explain why we use non-ssr optimized deps here. I'm actually not sure too 🤔
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.
We use them because in v2, we only had these deps. Actually we only need the list of deps for known imports. During ssr true we don't have that list, we only optimize the explicitly defined deps right now. And I don't think we should touch how legacy cjs externalization heuristics work
Description
Follow up to:
These changes were triggered by a report from @DylanPiercey while testing Vite v3 with Marko.
Refactor
Even if the PR looks big, the main change is a straight forward refactoring from:
to
We need this because we have now added more responsibilities to the optimizer, like tracking of the iddle state before optimizing. Having separate optimizers allows us to cleanly make the calls from the ssr pipeline noop. See later in the code:
Before this PR, there could be race conditions between the SSR requests and regular requests when registering ids in
delayDepsOptimizerUntil
.New experimental optimizeDeps.devSsr option
Edit: This is no longer needed, the optimizer is started on the first ssrLoadModule call
Before this PR, there was a simple condition to check if we should optimize deps for dev SSR:
This was generating runs of the dev SSR optimizer when using the Vue plugin, since it auto injects vue as
ssr.noExternal
.The PR right now adds an option to opt-in.
Note: I think we could change this to init the optimizer automatically on the first call to
ssrLoadModule
.Passing SSR state to esbuildDepPlugin
In SSR, before this PR we ended up with
browserExternalId
for node modules in CJS. Forwarding the ssr flag fixed this issue.Patching Esbuild dynamic require error
The last issue with the added test case seems to be an unresolved esbuild bug:
This PR implements the
banner
workaround mentioned here evanw/esbuild#1921 (comment). Interested in your feedback @benmccann since I saw you participating in that threadWhat is the purpose of this pull request?