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(ssr): support external true #10939

Merged
merged 9 commits into from
Jan 10, 2024
Merged

feat(ssr): support external true #10939

merged 9 commits into from
Jan 10, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Nov 15, 2022

Description

Support ssr.external: true to default all packages to external even if they are linked.

I found this handy in tests in monorepos where we want to emulate the usual case of all packages externaled, or when using ssrLoadModule to load an arbitrary file and we want to always external packages as we don't care about HMR.

Additional context

cc @brillout which brought up something similar some time ago #9367 (reply in thread)

re whether we want to change the default, perhaps we should see how SSR optimize deps help first in Vite 5 as we want to keep Vite 4 less breaking.

Notes on test: I made __tests__ a workspace package to unit test ssrExternal. Feels a bit weird but this avoids adding tests as e2e 😬


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bluwy bluwy added feat: ssr p2-to-be-discussed Enhancement under consideration (priority) labels Nov 15, 2022
.eslintrc.cjs Outdated Show resolved Hide resolved
Comment on lines -159 to -91
const { ssr } = config
if (ssr) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed const { ssr } = config as it's already define above. Removed if (ssr) { as it's always true.

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@brillout
Copy link
Contributor

re whether we want to change the default, perhaps we should see how SSR optimize deps help first in Vite 5 as we want to keep Vite 4 less breaking.

👍 Sounds like a good plan. As always, I strongly recommend to always externalize SSR dependencies by default. (Because SSR dependencies often crash when processed by a bundler. For example when they use import(someDynamicImportPath) which bundlers cannot statically analyze.)

docs/config/ssr-options.md Outdated Show resolved Hide resolved
docs/config/ssr-options.md Outdated Show resolved Hide resolved
@benmccann
Copy link
Collaborator

Just a note (mostly to myself): this is a bit different than defaulting everything to external because external: true causes noExternal to have no effect. That has historically meant you wouldn't be able to use external: true with vite-plugin-svelte since it needed to add Svelte libraries to noExternal. That may be different now that we've turned on prebundling, but you'd want to be careful not to both set external: true and prebundleSvelteLibraries: false

@bluwy
Copy link
Member Author

bluwy commented Nov 22, 2022

external: true causes noExternal to have no effect

I don't think this is what the PR does. noExternal will always have priority over external, unless if a package is explicitly defined in both external and noExternal (for some reason), external will take priority.

Explicitly defined means ssr.external: ['some-lib'] only, not ssr.external: true. If ssr.external: true and ssr.noExternal: true (for some reason), noExternal will still take priority.

@patak-dev patak-dev merged commit e451be2 into main Jan 10, 2024
10 checks passed
@patak-dev patak-dev deleted the ssr-external-true branch January 10, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p2-to-be-discussed Enhancement under consideration (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants