fix: externalize dependencies by default during SSR

[benmccann]

Description

Externalize dependencies by default during SSR

Additional context

We had talked about doing this in #5544. At the time, I didn't want to change the code more than necessary because we were trying to get 2.7 out the door and we had already made so many SSR changes. But now that SSR has settled down a bit, I do think it'd be a good cleanup to make.

As far as I'm aware, there's really nothing to gain by having complex logic to determine whether to bundle or externalize. I'm not aware of any case where that makes something work that otherwise wouldn't work. Rather, I'm only aware of cases where that breaks things because of incorrect or incomplete bundling logic (e.g. #2579)

There are libraries that don't work with Node.js because they're incorrectly packaged and would work if they were bundled since the bundler often fixes these issues. However, at the current moment, Vite's SSR bundling doesn't work well enough that bundling solves more problems than it creates. Problems created by bundling bugs are far harder to diagnose than incorrectly packaged Node modules. Even if we did want to bundle more aggressively, we should probably just do it by default for all packages rather than having the complex logic we have now.

One of our users recently reported a problem using reflect-metadata (sveltejs/kit#3334 (comment)). From a quick glance, it looks like the library is essentially a polyfill for a proposed piece of functionality to the ES spec, so it sticks its functionality onto the global scope and doesn't export anything. Because it doesn't export anything, we don't see exports or module.exports, etc. As a result, we fail to detect that the library is a CJS library and thus fail to externalize the library.

I had tried this earlier in #6698 but had a failing test. @ygj6 has fixed that in #8025. Unfortunately I'm unable to reopen the original PR, so am sending this new one

---

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.

[brillout]

Big 👍 for merging this from my side.

As far as I'm aware, there's really nothing to gain by having complex logic to determine whether to bundle or externalize. I'm not aware of any case where that makes something work that otherwise wouldn't work. Rather, I'm only aware of cases where that breaks things because of incorrect or incomplete bundling logic (e.g. #2579)

Same here. And I also agree on the rest.

I sent a change in #8033 to do this. It could theoretically be in a 2.9 release as it shouldn't be a breaking change. Thanks to ygj6 for figuring out the hard part and making it work!

I agree. Vite 3 is already big. AFAICT merging this in 2.9 seems like the good choice.

Thanks ygj6 for unlocking this.

[patak-dev]

We talked about the proposal yesterday with the team. It was good timing to do the PR because I think it may provoke bigger changes for v3.

The current external complex logic was introduced to cover the case of an ESM-only dependency. In #8025, @ygj6 transformed an ESM .js file to CJS. We shouldn't have done that, instead, we should have added a "type": "module" field to the dep package.json or to make that file .mjs. We revert and add that back in this PR to test. From the discussion, we think that this PR may end up failing in that case.

Instead of removing the logic, that was there because the SSR build defaults to CJS, in v3 we could introduce a breaking change and default the SSR build to ESM. We can do this now that node 14 will be the baseline. From ESM, we should be able to import both CJS and ESM without problems, so all the logic can be removed (maybe we could even simplify things more than in this PR?)

So the proposed plan for v3 is:

• SSR build is ESM, and we can remove the complex external logic
• There should be an option (new one or inferred from current ones) to opt-in to a CJS SSR build, where we would keep the current logic. We could still talk with others in the ecosystem to see if someone will end up using this mode, but it feels at this point that we should still provide it and maybe it can be completely removed in v4 next year.

@brillout @benmccann what do you think? @matthewp @frandiox also interested in your thoughts here

[benmccann]

instead, we should have added a "type": "module" field to the dep package.json

Hmm. I just tried that in #8060, but it's failing. Is this an issue with Jest? Maybe we can get rid of Jest now that Vitest is used for unit testing? #8040 I'd imagine Playwright could be used directly for integration testing without Jest being involved

in v3 we could introduce a breaking change and default the SSR build to ESM

Sounds good to me. We've been ESM-first in SvelteKit since the beginning and have run into very few problems. The only place I've seen that doesn't support ESM SSR is Netlify depending on which deployment option you use. They have two deployment methods: esbuild and zip-it-and-ship-it (zisi). The zisi deployment method only supports CJS for now. They're working on migrating that to ESM as well, but I don't necessarily expect it'd be done before v3. I would be fine to either temporarily drop zisi support or have SvelteKit run the Vite output through esbuild in a second step to convert it to CJS in that code path

[N0tExisting]

instead, we should have added a "type": "module" field to the dep package.json

This would cause issues if you only copy the build output and then separately install (for example without the dev Dependencies)

If we need CJS compat there exists createRequire in node:module

[ygj6]

Is this an issue with Jest?

Yes, I tried to run it (#8060) locally instead of testing it and it is ok.

[brillout]

From my perspective this is a solid plan.

Only thing: I think we can drop CJS application code already in v3. In theory, there shouldn't be any problem. In practice, other than Netlify's zisi thing, I'm not aware of any problems.

Worst case, as Ben said, the user can use esbuild to convert ESM to CJS.

All in all, it seems to me we can already go for only outputting ESM application code for v3.

Also, it prepares us for Kill CJS #5839: if, after a while, we see that there is no need for CJS application code, then we can ship Vite as ESM-only.

[matthewp]

We use ESM for SSR builds in Astro. The biggest problems we run into is some packages that use non-standard package.json fields like "module" that are not supported by raw Node.js ESM, which leads to dev/prod differences.

[frandiox]

We also mainly use ESM for SSR builds in both Hydrogen and Vitedge. We still need the noExternal: true when building for workers but I think that's not a problem here, right?
In fact, we recently tried to drop CJS completely in Hydrogen but ran into some issues.

[benmccann]

#8060 has been merged to revert the test change as suggested above (#8033 (comment)), so this PR is now failing. We'll need to switch the build to ESM first

With SvelteKit, Astro, Nuxt, vite-plugin-ssr, Hydrogen, and Vitedge all using ESM already maybe we could stop supporting CJS altogether

[matthewp]

@benmccann how so, removing rollupOptions?

[benmccann]

I may not be the best person to ask, but from my quick glance I think it may be this line:

vite/packages/vite/rollup.config.js

Line 72 in 274c10e

format: 'cjs',

[benmccann]

I filed an issue for outputting ESM for the SSR build: #8150. If anyone here would like to send a PR, feel free as I won't be able to.

[patak-dev]

With SvelteKit, Astro, Nuxt, vite-plugin-ssr, Hydrogen, and Vitedge all using ESM already maybe we could stop supporting CJS altogether

We talked today again with the team, and given the response from the ecosystem, it is clear that ESM should be the default for SSR. Together with Anthony's PR to move Vite build to ESM (with a minimal CJS compat layer), Vite v3 would be more aligned with the rest of the ecosystem and already doing a big step toward ESM adoption. After ESM SSR proves to be stable we can also remove the SSR experimental status (that we should probably still keep in v3 given this change).
We still think that we should keep the current CJS SSR as an option though (in case it is needed by some users since all the work is already done), although all the promotion would be towards people using ESM SSR builds.

[patak-dev]

Let's close this PR, as we are going to keep this logic for the opt-in SSR CJS build. We can continue the discussion at:

• Use ESM for SSR builds #8150