-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Changes from 9 commits
3b08d34
7aa0b7a
92cfb81
8dc7d98
212f81a
ac494ac
ef6e8ea
2923000
d8adf62
ad9f17e
773f234
b71e1ea
5ce5087
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { defineConfig } from 'vite' | ||
|
||
export default defineConfig({ | ||
ssr: { | ||
target: 'node-cjs' | ||
} | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,10 @@ import { manifestPlugin } from './plugins/manifest' | |
import type { Logger } from './logger' | ||
import { dataURIPlugin } from './plugins/dataUri' | ||
import { buildImportAnalysisPlugin } from './plugins/importAnalysisBuild' | ||
import { resolveSSRExternal, shouldExternalizeForSSR } from './ssr/ssrExternal' | ||
import { | ||
cjsShouldExternalizeForSSR, | ||
cjsSsrResolveExternals | ||
} from './ssr/ssrExternal' | ||
import { ssrManifestPlugin } from './ssr/ssrManifestPlugin' | ||
import type { DepOptimizationMetadata } from './optimizer' | ||
import { | ||
|
@@ -373,27 +376,14 @@ async function doBuild( | |
ssr ? config.plugins.map((p) => injectSsrFlagToHooks(p)) : config.plugins | ||
) as Plugin[] | ||
|
||
// inject ssrExternal if present | ||
const userExternal = options.rollupOptions?.external | ||
let external = userExternal | ||
if (ssr) { | ||
// see if we have cached deps data available | ||
let knownImports: string[] | undefined | ||
const dataPath = path.join(getDepsCacheDir(config), '_metadata.json') | ||
try { | ||
const data = JSON.parse( | ||
fs.readFileSync(dataPath, 'utf-8') | ||
) as DepOptimizationMetadata | ||
knownImports = Object.keys(data.optimized) | ||
} catch (e) {} | ||
if (!knownImports) { | ||
// no dev deps optimization data, do a fresh scan | ||
knownImports = await findKnownImports(config) | ||
} | ||
external = resolveExternal( | ||
resolveSSRExternal(config, knownImports), | ||
userExternal | ||
) | ||
|
||
// In CJS, we can pass the externals to rollup as is. In ESM, we need to | ||
// do it in the resolve plugin so we can add the resolved extension for | ||
// deep node_modules imports | ||
if (ssr && config.ssr?.target === 'node-cjs') { | ||
external = await cjsSsrResolveExternal(config, userExternal) | ||
} | ||
|
||
if (isDepsOptimizerEnabled(config) && !ssr) { | ||
|
@@ -431,10 +421,12 @@ async function doBuild( | |
|
||
try { | ||
const buildOutputOptions = (output: OutputOptions = {}): OutputOptions => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if the input is There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
const cjsSsrBuild = ssr && config.ssr?.target === 'node-cjs' | ||
return { | ||
dir: outDir, | ||
format: ssr ? 'cjs' : 'es', | ||
exports: ssr ? 'named' : 'auto', | ||
// Default format is 'es' for regular and for SSR builds | ||
format: cjsSsrBuild ? 'cjs' : 'es', | ||
exports: cjsSsrBuild ? 'named' : 'auto', | ||
sourcemap: options.sourcemap, | ||
name: libOptions ? libOptions.name : undefined, | ||
generatedCode: 'es2015', | ||
|
@@ -696,26 +688,51 @@ export function onRollupWarning( | |
} | ||
} | ||
|
||
function resolveExternal( | ||
ssrExternals: string[], | ||
async function cjsSsrResolveExternal( | ||
config: ResolvedConfig, | ||
user: ExternalOption | undefined | ||
): ExternalOption { | ||
): Promise<ExternalOption> { | ||
// see if we have cached deps data available | ||
let knownImports: string[] | undefined | ||
const dataPath = path.join(getDepsCacheDir(config), '_metadata.json') | ||
try { | ||
const data = JSON.parse( | ||
fs.readFileSync(dataPath, 'utf-8') | ||
) as DepOptimizationMetadata | ||
knownImports = Object.keys(data.optimized) | ||
} catch (e) {} | ||
if (!knownImports) { | ||
// no dev deps optimization data, do a fresh scan | ||
knownImports = await findKnownImports(config) | ||
} | ||
const ssrExternals = cjsSsrResolveExternals(config, knownImports) | ||
|
||
return (id, parentId, isResolved) => { | ||
if (shouldExternalizeForSSR(id, ssrExternals)) { | ||
const isExternal = cjsShouldExternalizeForSSR(id, ssrExternals) | ||
if (isExternal) { | ||
return true | ||
} | ||
if (user) { | ||
if (typeof user === 'function') { | ||
return user(id, parentId, isResolved) | ||
} else if (Array.isArray(user)) { | ||
return user.some((test) => isExternal(id, test)) | ||
} else { | ||
return isExternal(id, user) | ||
} | ||
return resolveUserExternal(user, id, parentId, isResolved) | ||
} | ||
} | ||
} | ||
|
||
function resolveUserExternal( | ||
user: ExternalOption, | ||
id: string, | ||
parentId: string | undefined, | ||
isResolved: boolean | ||
) { | ||
if (typeof user === 'function') { | ||
return user(id, parentId, isResolved) | ||
} else if (Array.isArray(user)) { | ||
return user.some((test) => isExternal(id, test)) | ||
} else { | ||
return isExternal(id, user) | ||
} | ||
} | ||
|
||
function isExternal(id: string, test: string | RegExp) { | ||
if (typeof test === 'string') { | ||
return id === test | ||
|
There was a problem hiding this comment.
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 withssr.lazy: false
?There was a problem hiding this comment.
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 notssr.target
There was a problem hiding this comment.
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 inssr.target
and let users control the format and eager externalization usingbuild.rollupOptions.output.format: 'cjs'
.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 havessr.target
as an enum, I think that adding anode-cjs
orcjs
is ok. Although another option likessr.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.
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 alsossr.scan
andssr.disabled: 'build'
for example)See #8348 (comment)