feat: default esm SSR build, simplified externalization #8348
Conversation
|
Tests are failing because of VitePress, I imagine should now set the format to |
docs/config/ssr-options.md
Outdated
| @@ -20,7 +20,7 @@ Prevent listed dependencies from being externalized for SSR. If `true`, no depen | |||
|
|
|||
| ## ssr.target | |||
|
|
|||
| - **Type:** `'node' | 'webworker'` | |||
| - **Type:** `'node' | 'webworker' | 'node-cjs'` | |||
There was a problem hiding this comment.
We could also use build.rollupOptions.output.format === 'cjs', but we need this target during dev for the new externalization logic. We could add a new option to switch externalization logic if not. ssr.lazy: true? And then people can choose to go back to the old eager collection with ssr.lazy: false?
There was a problem hiding this comment.
Could the externalization logic also look at build.rollupOptions.output.format? Having two options seems a bit dangerous because someone could change build.rollupOptions.output.format, but not ssr.target
There was a problem hiding this comment.
Maybe we could, since we want to promote the ESM build and the new externalization logic, we don't really have a new Vite option here... it is more like a escape hatch that we're leaving for people that can yet use the ESM build. If we do that, we should remove the new 'node-cjs' option in ssr.target and let users control the format and eager externalization using build.rollupOptions.output.format: 'cjs'.
There was a problem hiding this comment.
Same opinion as Ben: a single source of truth would be nice and build.rollupOptions.output.format seems like a natural fit.
There was a problem hiding this comment.
I don't think you can use build.rollupOptions.output specifically, since it may be an array?
There was a problem hiding this comment.
Yes, and there are other issues with it, like needing to conditionally change it based on the ssr flag. But this flag isn't currently passed to the ConfigEnv. We could add it so we could configure using ({ mode, command, ssr }) => { ... }), but forcing a conditional as the only way to configure this doesn't seem very appealing. For other options, we discussed previously with @aleclarson and tried to avoid having SSR only equivalents, but this is a big global switch. Given that we already have ssr.target as an enum, I think that adding a node-cjs or cjs is ok. Although another option like ssr.format: 'esm' | 'cjs' could be easier to deprecate in the future... we could add it as @experimental, and give us room to remove it later on.
There was a problem hiding this comment.
Changed to ssr.format: 'cjs', we'll later review all these new options together (there is also ssr.scan and ssr.disabled: 'build' for example)
See #8348 (comment)
| return (id: string) => { | ||
| if (processedIds.has(id)) { | ||
| return processedIds.get(id) | ||
| } | ||
| const external = | ||
| isBuiltin(id) || (isPackageEntry(id) && isConfiguredAsExternal(id)) | ||
| processedIds.set(id, external) | ||
| return external | ||
| } |
There was a problem hiding this comment.
This is the new lazy externalization logic.
|
Fixed VitePress by using WIP for ssr-react (some tests should fail, but pushing in case someone else is checking out the PR) It looks like the issue is that in CJS, we can require deep imports without the extension. But with ESM, we need to import these using the extension. This is the error when running the generated ssr-react entry-server.js Instead of giving rollup an externals function, we can define that an id is external in The commit also removes the |
|
The only reason I can think of for having that node_modules check is for
linking and then developing dependencies, but I believe we have more
reasons for externalizing linked dependencies.
As we have seen some Node.js libraries cause problems and, in general, we
are moving towards always externalizing dependencies by default.
The same arguments that make us move towards externalizing also holds for
linked dependencies. (We cannot and should not assume that Vite is able to
process all Node.js libraries out there, e.g. libraries with dynamic
require/import that Vite cannot statically determine.)
For those who want to use Vite to develop linked dependencies they can use
vite.config.js#ssr.noExternal.
CC @aleclarson seemed opinionated about this topic.
CC @benmccann
…On Fri 27. May 2022 at 23:12, patak ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/vite/src/node/ssr/ssrExternal.ts
<#8348 (comment)>:
> + if (
+ tryNodeResolve(
+ id,
+ undefined,
+ resolveOptions,
+ ssr?.target === 'webworker',
+ undefined,
+ true
+ )
+ ) {
+ return true
+ }
+ try {
+ // no main entry, but deep imports may be allowed
+ const pkgPath = resolveFrom(`${id}/package.json`, root)
+ if (pkgPath.includes('node_modules')) {
Yes, I copied this code from the previous logic, but now it is a good time
to change things if we see fit. We can try to simplify this even more and
then add back logic if there are issues.
—
Reply to this email directly, view it on GitHub
<#8348 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHVQRTXGF3E6RWH77UI4TLVME3CTANCNFSM5XBSWQOQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I think now and vite/packages/vite/src/node/plugins/workerImportMetaUrl.ts Lines 93 to 98 in 2923000 when target is not |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
ok, |
|
@brillout I agree with your assessment FWIW, this is what Saus uses to determine if an SSR module should be compiled by Vite: const isCompiledModule = (id: string) =>
!id.includes('/node_modules/') && id.startsWith(root + '/')So technically, if a linked package exists in the project root (but not within node_modules), it will be compiled by Vite. Not exactly sure how perfect this heuristic works when using a monorepo root as |
|
@brillout @aleclarson thanks for reviewing that logic. In the current CJS externalization heuristic, we're currently doing: try {
// no main entry, but deep imports may be allowed
const pkgPath = resolveFrom(`${id}/package.json`, root)
if (pkgPath.includes('node_modules')) {
ssrExternals.add(id)
} else {
depsToTrace.add(path.dirname(pkgPath))
}
continue
} catch {}So the dep isn't added to ssrExternals if it isn't in node_modules, but the it still added to depsToTrace... and I think it ends up traced later on. @dominikg is also reporting issues with linked dependencies that may be related. I think that we an opportunity to try @brillout's proposal here. Let's remove the |
This is great @poyoho, let's merge first this PR, and you can later send a new one so we also add docs and a test case. Good catch, the ESM SSR build will clean up a lot of these restrictions. |
|
Updated the PR to use I think we should merge this PR, and we can later discuss the naming (or the need for the option to go back to CJS in a team meeting together with other experimental options we've been adding). The SSR build is still using plugin common-js, I'll try to remove that in another PR. |
| @@ -431,10 +421,12 @@ async function doBuild( | |||
|
|
|||
| try { | |||
| const buildOutputOptions = (output: OutputOptions = {}): OutputOptions => { | |||
There was a problem hiding this comment.
Even if the input is .mjs we still emits .js. I am not sure if we should always emit .mjs on ESM build, or respect the input (might need to also convert .mts)
There was a problem hiding this comment.
Maybe we should do the same we are doing in v3 for lib mode?
There was a problem hiding this comment.
I'll merge this one and work on this logic in another PR.
sapphi-red
added
enhancement
|
LGTM. I'm thrilled we are now always externalizing by default. |
Thanks for pushing for externalizing by default all this time, we needed a bit of time for all the pieces to fall into place 🙂 |
Close #8150
Fix #2152
Close #2157
Close #6812
Should close #6023
Description
For context, check:
Initial work towards defaulting to ESM SSR by default. Still WIP. I'm hoping to get some help from folks that are more deep into the weeds here (ping @benmccann, @brillout, @antfu, @bluwy, just to name a few, any feedback welcome)
Since we move to ESM builds, SSR tests have been moved to
"type": "module"now. See related work for non SSR tests started by Anthony here:type: module#8252You can check the changesets independently:
There is a new
ssr.targetoption callednode-cjs(and by default thenodetarget will generate a ESM build).The ssr-react playground is still using a CJS SSR build because it is failing when rollup leaves the CJS only packages as external without interop. Someone could check what is missing?
If the SSR target is
node-cjs, the externalize logic has been left untouched. It still uses the scanner to find known imports, and collect all externals eagerly. Since we would like to push people to use the ESM version, I don't think we should refactor the way it works now. I renamedshouldExternalizeForSSRtocjsShouldExternalizeForSSR, and keep the new functions clean.For ESM build, the externalization logic is now done lazily (there was a comment in the code already asking why we weren't doing so). Right now the logic may be a bit simplistic (check if it is a package entry and in that case if it should be externalized according to
ssr.external,ssr.noExternal, with external being the default).What is the purpose of this pull request?