Skip to content
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

Merged
merged 5 commits into from Jun 29, 2022

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Jun 29, 2022

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:

getDepsOptimizer(config)?.metadata({ ssr })

to

getDepsOptimizer(config, { ssr }).metadata

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:

    // noop, there is no scanning during dev SSR
    // the optimizer blocks the server start
    run: () => {},
    registerWorkersSource: (id: string) => {},
    delayDepsOptimizerUntil: (id: string, done: () => Promise<any>) => {},
    resetRegisteredIds: () => {},
    ensureFirstRun: () => {},

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:

if (!isBuild && config.ssr) {
  ssrServerDepsMetadata = await optimizeServerSsrDeps(config)
}

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 thread


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@netlify
Copy link

netlify bot commented Jun 29, 2022

Deploy Preview for vite-docs-main ready!

Name Link
🔨 Latest commit c043ebe
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62bcc5fa8e1a19000936258e
😎 Deploy Preview https://deploy-preview-8854--vite-docs-main.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@patak-dev patak-dev changed the title fix: optimizeDeps.ssrDev, builtin imports in node fix: optimize deps on dev SSR, builtin imports in node Jun 29, 2022
@patak-dev
Copy link
Member Author

Remove the new optimizeDeps.devSsr option in:

@patak-dev patak-dev requested a review from bluwy June 29, 2022 12:45
@patak-dev patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label Jun 29, 2022
@patak-dev patak-dev added this to the 3.0 milestone Jun 29, 2022
@patak-dev patak-dev added feat: deps optimizer Esbuild Dependencies Optimization feat: ssr labels Jun 29, 2022
@patak-dev
Copy link
Member Author

vite-ecosystem-ci run against this PR https://github.com/vitejs/vite-ecosystem-ci/actions/runs/2583218491

Copy link
Member

@bluwy bluwy left a 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!

Comment on lines +567 to +572
banner:
platform === 'node'
? {
js: `import { createRequire } from 'module';const require = createRequire(import.meta.url);`
}
: undefined,
Copy link
Member

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.

Comment on lines 763 to 764
// Important! We use the non-ssr optimized deps to find known imports
const depsOptimizer = getDepsOptimizer(server.config, { ssr: false })
Copy link
Member

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 🤔

Copy link
Member Author

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

packages/vite/src/node/optimizer/optimizer.ts Show resolved Hide resolved
@patak-dev patak-dev merged commit d49856c into main Jun 29, 2022
@patak-dev patak-dev deleted the fix/optimize-deps-on-dev-ssr-opt-in branch June 29, 2022 21:48
@patak-dev patak-dev mentioned this pull request Jul 4, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: deps optimizer Esbuild Dependencies Optimization feat: ssr p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants