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: default esm SSR build, simplified externalization #8348

Merged
merged 13 commits into from
May 29, 2022

Conversation

patak-dev
Copy link
Member

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:

You can check the changesets independently:

There is a new ssr.target option called node-cjs (and by default the node target 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 renamed shouldExternalizeForSSR to cjsShouldExternalizeForSSR, 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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev
Copy link
Member Author

Tests are failing because of VitePress, I imagine should now set the format to node-cjs 🤔

@@ -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'`
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 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?

Copy link
Collaborator

@benmccann benmccann May 26, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same opinion as Ben: a single source of truth would be nice and build.rollupOptions.output.format seems like a natural fit.

Copy link
Member

@aleclarson aleclarson May 28, 2022

Choose a reason for hiding this comment

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

I don't think you can use build.rollupOptions.output specifically, since it may be an array?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Comment on lines 168 to 176
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
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the new lazy externalization logic.

@patak-dev
Copy link
Member Author

Fixed VitePress by using ssr.target: 'node-cjs' for it:

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

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/patak/vite-ssr/playground-temp/ssr-react/node_modules/react-dom/server' imported from /Users/patak/vite-ssr/playground-temp/ssr-react/dist/server/entry-server.js
Did you mean to import [email protected][email protected]/node_modules/react-dom/server.js?

Instead of giving rollup an externals function, we can define that an id is external in resolvedId of resolve.ts, where we have the logic to know if the import is a deep import and check if the extension needs to be added to the id. This seems to work for ssr-react, the build is working. There are still some tests failing (and a new one in ssr-vue), so still WIP.

The commit also removes the ssrRequireHookPlugin for the ESM build.

@patak-dev patak-dev marked this pull request as draft May 26, 2022 22:12
@patak-dev patak-dev marked this pull request as ready for review May 27, 2022 09:46
@brillout
Copy link
Contributor

brillout commented May 28, 2022 via email

@poyoho
Copy link
Member

poyoho commented May 28, 2022

I think now new URL(url, import.meta.url) syntax is allow in SSR. So can we remove

and

if (options?.ssr) {
this.error(
`\`new URL(url, import.meta.url)\` is not supported in SSR.`,
urlIndex
)
}

when target is not node-cjs 🤔

@userquin

This comment was marked as resolved.

@userquin

This comment was marked as off-topic.

@userquin
Copy link
Contributor

ok, pnpm run test now ok on Windows 10 PRO 🚀 : with core.symliks=true (GIT), process.exit(0) building Vite (scripts/patchCJS.ts) and 0b054fc applied (playground/vitestSetup.ts)

@aleclarson
Copy link
Member

@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 viteConfig.root whilst also having pre-compiled packages in the same monorepo (but it's probably no biggie).

@patak-dev
Copy link
Member Author

@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 node_modules check, and we can add it back when we have a bug report to justify it. Done in fix: externalize packages out of node_modules

@patak-dev
Copy link
Member Author

I think now new URL(url, import.meta.url) syntax is allow in SSR. So can we remove

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.

@patak-dev
Copy link
Member Author

Updated the PR to use ssr.format: 'cjs' instead of ssr.target: 'node-cjs'. See rationale here #8348 (comment)

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 => {
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@patak-dev patak-dev May 29, 2022

Choose a reason for hiding this comment

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

Maybe we should do the same we are doing in v3 for lib mode?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll merge this one and work on this logic in another PR.

antfu
antfu previously approved these changes May 29, 2022
Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

Other than my previous comment, it LGTM 🔥

@sapphi-red sapphi-red added enhancement New feature or request feat: ssr p3-significant High priority enhancement (priority) labels May 29, 2022
@sapphi-red sapphi-red added this to the 3.0 milestone May 29, 2022
@patak-dev patak-dev merged commit f8c92d1 into main May 29, 2022
@patak-dev patak-dev deleted the feat/esm-ssr-build branch May 29, 2022 13:09
@brillout
Copy link
Contributor

LGTM. I'm thrilled we are now always externalizing by default.

@patak-dev
Copy link
Member Author

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 🙂

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

Successfully merging this pull request may close these issues.

Use ESM for SSR builds Feature: Allow SSR build to output ES
8 participants