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

feat: support cjs noExternal in SSR dev, fix #2579 #8430

Merged
merged 6 commits into from
Jun 18, 2022

Conversation

patak-dev
Copy link
Member

Description

Fix #2579

See the issue for context, in particular, this comment:
#2579 (comment)

After the latest changes to use the esbuild deps optimizer during build, the only place where wasn't still used was during dev ssr.

If a dependency like 'cookie' that is using exports instead of module.exports and was marked as noExternal, Vite couldn't process it during dev ssr. The dependency could be marked as external but there may be cases where the user would like to generate a bundled SSR build.

This PR creates a dedicated optimized deps cache for dev ssr when the server starts, and allow these dependencies that are now working well during build to also work during dev. We don't yet have a discovery mechanism of deps during dev ssr and adding it would also imply adding a way to invalidate a ssrLoadModule request and retry it if the optimized deps changed (equivalent to a full reload in the browser). So the PR only is currently only optimizing dependencies that are both marked as noExternal and explicitly added to the optimizeDeps.include. I think this may be enough, as it acts more as a way to overcome issues with some dependencies.

Sending the PR as is to start the discussion, I still don't know if this is the best approach moving forward but it may be a way out of one of the most voted issues in Vite and it is also good that dev ssr for these deps will be closer to build ssr.

There isn't a config option to disable this new optimization step, I think we may not need one here.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev added feat: ssr p3-significant High priority enhancement (priority) labels Jun 1, 2022
@patak-dev
Copy link
Member Author

We could also automatically include every explicit (type 'string') dependency in noExternal as included for optimization in SSR dev so there is no need to repeat the dep in the config in most cases

@patak-dev patak-dev requested a review from antfu June 1, 2022 11:17
@benmccann
Copy link
Collaborator

Wow, this will make a lot of people very happy!!

The PR only is currently only optimizing dependencies that are both marked as noExternal and explicitly added to the optimizeDeps.include

This probably works well enough with one caveat. You can specify values in noExternal that you can't in optimizeDeps.include. E.g. one of the Svelte maintainers who is the biggest proponent of bundling uses a regex to bundle everything except for one library and noExternal accepts a regex but optimizeDeps.include does not.

(On a related note, I think the precedence of ssr.external/ssr.noExternal might be non-optimal. If I recall correctly, noExternal takes precedence. But everything is externalized by default making the external flag a bit unnecessary except as an override to noExternal. E.g. it'd be nice if you specify noExternal: true to be able to exclude a library with external, but if my memory is correct you can't do that today)

@patak-dev
Copy link
Member Author

This probably works well enough with one caveat. You can specify values in noExternal that you can't in optimizeDeps.include. E.g. one of the Svelte maintainers who is the biggest proponent of bundling uses a regex to bundle everything except for one library and noExternal accepts a regex but optimizeDeps.include does not.

Yes, in the PR I'm only auto including every noExternal entry that is a string. If someone use a regex, they will need to manually include the CJS deps in include. I think that could be ok for now, no? We could have a discovery scheme at one point but I'm unsure if the added complexity is worthy at this point.

(On a related note, I think the precedence of ssr.external/ssr.noExternal might be non-optimal. If I recall correctly, noExternal takes precedence. But everything is externalized by default making the external flag a bit unnecessary except as an override to noExternal. E.g. it'd be nice if you specify noExternal: true to be able to exclude a library with external, but if my memory is correct you can't do that today)

I already changed it to work as you expect in v3, see

if (!ssr || ssr.external?.includes(id)) {

I kind of consider this a fix more than a breaking change. An explicit external should take precedence over noExternal.

bluwy
bluwy previously approved these changes Jun 13, 2022
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.

So the PR only is currently only optimizing dependencies that are both marked as noExternal and explicitly added to the optimizeDeps.include. I think this may be enough, as it acts more as a way to overcome issues with some dependencies.

I think this makes sense to me. Perhaps we can have a separate PR to improve ssrLoadModule to error on CJS code and recommend this option, but otherwise it's good that users have a way around the issue.

@patak-dev
Copy link
Member Author

So the PR only is currently only optimizing dependencies that are both marked as noExternal and explicitly added to the optimizeDeps.include. I think this may be enough, as it acts more as a way to overcome issues with some dependencies.

I think this makes sense to me. Perhaps we can have a separate PR to improve ssrLoadModule to error on CJS code and recommend this option, but otherwise it's good that users have a way around the issue.

Just for reference, the logic now auto include each explicit noExternal entry that isn't in optimizeDeps.exclude. This avoids the need to duplicate the entries in the config file. There may be more optimized deps that aren't strictly needed but I think this is ok as it makes dev SSR works closer to other modes for these deps.

@bluwy maybe we should merge this PR so people can test it in an alpha. Worst case we disable the feature if we decide in a team meeting that this isn't a good idea. What do you think?

@bluwy
Copy link
Member

bluwy commented Jun 13, 2022

Just for reference, the logic now auto include each explicit noExternal entry that isn't in optimizeDeps.exclude. This avoids the need to duplicate the entries in the config file. There may be more optimized deps that aren't strictly needed but I think this is ok as it makes dev SSR works closer to other modes for these deps.

Ah I didn't notice that. Yeah I think it should be fine then. +1 on merging this for alpha

@patak-dev
Copy link
Member Author

We discussed this PR with Evan and we are good to merge it. Let's get it out in the beta so we can resolve #2579

@patak-dev patak-dev changed the title feat: support cjs noExternal in SSR dev feat: support cjs noExternal in SSR dev, fix #2579 Jun 18, 2022
@patak-dev patak-dev merged commit 11d2191 into main Jun 18, 2022
@patak-dev patak-dev deleted the feat/optimize-deps-in-dev-ssr branch June 18, 2022 08:32
@aleclarson
Copy link
Member

Saus needs an option to disable tryOptimizedResolve calls for SSR modules.

@patak-dev What would you prefer the option name be?

@aleclarson
Copy link
Member

Oh it appears that I just have to not call server.ssrLoadModule and I'll be fine (in dev mode at least).

if (isDepsOptimizerEnabled(config, true)) {
await initDevSsrDepsOptimizer(config, server)
}

But it looks like ssr.optimizeDeps.disabled is what I'm looking for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bundling SSR Modules using CommonJS exports alias results in ReferenceError: exports is not defined
4 participants