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
2 changes: 1 addition & 1 deletion docs/config/ssr-options.md
Expand Up @@ -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

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)

- **Default:** `node`

Build target for the SSR server.
102 changes: 72 additions & 30 deletions packages/vite/src/node/build.ts
Expand Up @@ -31,7 +31,11 @@ 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,
shouldExternalizeForSSR
} from './ssr/ssrExternal'
import { ssrManifestPlugin } from './ssr/ssrManifestPlugin'
import type { DepOptimizationMetadata } from './optimizer'
import { findKnownImports, getDepsCacheDir } from './optimizer'
Expand Down Expand Up @@ -367,27 +371,10 @@ 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
)
external = await ssrResolveExternal(config, userExternal)
}

const rollupOptions: RollupOptions = {
Expand Down Expand Up @@ -421,10 +408,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.

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',
Expand Down Expand Up @@ -686,26 +675,79 @@ export function onRollupWarning(
}
}

function resolveExternal(
async function ssrResolveExternal(
config: ResolvedConfig,
user: ExternalOption | undefined
): Promise<ExternalOption> {
if (config.ssr?.target !== 'node-cjs') {
return esmSsrResolveExternal(config, user)
} else {
// 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)
}
return cjsSsrResolveExternal(
cjsSsrResolveExternals(config, knownImports),
user
)
}
}

function esmSsrResolveExternal(
config: ResolvedConfig,
user: ExternalOption | undefined
): ExternalOption {
return (id, parentId, isResolved) => {
if (user) {
const isUserExternal = resolveUserExternal(user, id, parentId, isResolved)
if (typeof isUserExternal === 'boolean') {
return isUserExternal
}
}
return shouldExternalizeForSSR(id, config)
}
}

// When ssr.format is node-cjs, this function reverts back to the 2.9 logic for externalization
function cjsSsrResolveExternal(
ssrExternals: string[],
user: ExternalOption | undefined
): ExternalOption {
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
Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/config.ts
Expand Up @@ -223,7 +223,7 @@ export interface ExperimentalOptions {
importGlobRestoreExtension?: boolean
}

export type SSRTarget = 'node' | 'webworker'
export type SSRTarget = 'node' | 'webworker' | 'node-cjs'

export interface SSROptions {
external?: string[]
Expand Down
14 changes: 9 additions & 5 deletions packages/vite/src/node/plugins/importAnalysis.ts
Expand Up @@ -41,7 +41,10 @@ import {
} from '../utils'
import type { ResolvedConfig } from '../config'
import type { Plugin } from '../plugin'
import { shouldExternalizeForSSR } from '../ssr/ssrExternal'
import {
cjsShouldExternalizeForSSR,
shouldExternalizeForSSR
} from '../ssr/ssrExternal'
import { transformRequest } from '../server/transformRequest'
import {
getDepsCacheDir,
Expand Down Expand Up @@ -363,10 +366,11 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
}
// skip ssr external
if (ssr) {
if (
server._ssrExternals &&
shouldExternalizeForSSR(specifier, server._ssrExternals)
) {
if (config.ssr?.target === 'node-cjs') {
if (cjsShouldExternalizeForSSR(specifier, server._ssrExternals)) {
continue
}
} else if (shouldExternalizeForSSR(specifier, config)) {
continue
}
if (isBuiltin(specifier)) {
Expand Down
4 changes: 2 additions & 2 deletions packages/vite/src/node/server/index.ts
Expand Up @@ -22,7 +22,7 @@ import {
resolveHostname
} from '../utils'
import { ssrLoadModule } from '../ssr/ssrModuleLoader'
import { resolveSSRExternal } from '../ssr/ssrExternal'
import { cjsSsrResolveExternals } from '../ssr/ssrExternal'
import {
rebindErrorStacktrace,
ssrRewriteStacktrace
Expand Down Expand Up @@ -339,7 +339,7 @@ export async function createServer(
...Object.keys(optimizedDeps.metadata.discovered)
]
}
server._ssrExternals = resolveSSRExternal(config, knownImports)
server._ssrExternals = cjsSsrResolveExternals(config, knownImports)
}
return ssrLoadModule(
url,
Expand Down
114 changes: 107 additions & 7 deletions packages/vite/src/node/ssr/ssrExternal.ts
Expand Up @@ -5,7 +5,9 @@ import { createFilter } from '@rollup/pluginutils'
import type { InternalResolveOptions } from '../plugins/resolve'
import { tryNodeResolve } from '../plugins/resolve'
import {
bareImportRE,
createDebugger,
isBuiltin,
isDefined,
lookupFile,
normalizePath,
Expand All @@ -29,7 +31,7 @@ export function stripNesting(packages: string[]) {
* Heuristics for determining whether a dependency should be externalized for
* server-side rendering.
*/
export function resolveSSRExternal(
export function cjsSsrResolveExternals(
config: ResolvedConfig,
knownImports: string[]
): string[] {
Expand All @@ -49,7 +51,7 @@ export function resolveSSRExternal(
seen.add(id)
})

collectExternals(
cjsSsrCollectExternals(
config.root,
config.resolve.preserveSymlinks,
ssrExternals,
Expand Down Expand Up @@ -86,8 +88,97 @@ const CJS_CONTENT_RE =
// TODO: use import()
const _require = createRequire(import.meta.url)

// do we need to do this ahead of time or could we do it lazily?
function collectExternals(
const isSsrExternalCache = new WeakMap<
ResolvedConfig,
(id: string) => boolean | undefined
>()

export function shouldExternalizeForSSR(
id: string,
config: ResolvedConfig
): boolean | undefined {
let isSsrExternal = isSsrExternalCache.get(config)
if (!isSsrExternal) {
isSsrExternal = createIsSsrExternal(config)
isSsrExternalCache.set(config, isSsrExternal)
}
return isSsrExternal(id)
}

function createIsSsrExternal(
config: ResolvedConfig
): (id: string) => boolean | undefined {
const processedIds = new Map<string, boolean | undefined>()

const { ssr, root } = config

const noExternal = ssr?.noExternal
const noExternalFilter =
noExternal !== 'undefined' &&
typeof noExternal !== 'boolean' &&
createFilter(undefined, noExternal, { resolve: false })

const isConfiguredAsExternal = (id: string) => {
const { ssr } = config
if (!ssr || ssr.external?.includes(id)) {
return true
}
if (typeof noExternal === 'boolean') {
return !noExternal
}
if (noExternalFilter) {
return noExternalFilter(id)
}
return true
}

const resolveOptions: InternalResolveOptions = {
root,
preserveSymlinks: config.resolve.preserveSymlinks,
isProduction: false,
isBuild: true
}

const isPackageEntry = (id: string) => {
if (!bareImportRE.test(id) || id.includes('\0')) {
return false
}
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')) {
patak-dev marked this conversation as resolved.
Show resolved Hide resolved
return true
}
} catch {}
return false
}

return (id: string) => {
if (processedIds.has(id)) {
return processedIds.get(id)
}
const external =
isBuiltin(id) || (isPackageEntry(id) && isConfiguredAsExternal(id))
patak-dev marked this conversation as resolved.
Show resolved Hide resolved
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.

}

// When ssr.format is 'node-cjs', this function is used reverting to the Vite 2.9 era
// SSR externalize heuristics
function cjsSsrCollectExternals(
root: string,
preserveSymlinks: boolean | undefined,
ssrExternals: Set<string>,
Expand Down Expand Up @@ -192,14 +283,23 @@ function collectExternals(
}

for (const depRoot of depsToTrace) {
collectExternals(depRoot, preserveSymlinks, ssrExternals, seen, logger)
cjsSsrCollectExternals(
depRoot,
preserveSymlinks,
ssrExternals,
seen,
logger
)
}
}

export function shouldExternalizeForSSR(
export function cjsShouldExternalizeForSSR(
id: string,
externals: string[]
externals: string[] | null
): boolean {
if (!externals) {
return false
}
const should = externals.some((e) => {
if (id === e) {
return true
Expand Down
2 changes: 1 addition & 1 deletion playground/ssr-deps/__tests__/serve.ts
Expand Up @@ -10,7 +10,7 @@ export const port = ports['ssr-deps']
export async function serve() {
await kill(port)

const { createServer } = require(path.resolve(rootDir, 'server.js'))
const { createServer } = await import(path.resolve(rootDir, 'server.js'))
const { app, vite } = await createServer(rootDir, hmrPorts['ssr-deps'])

return new Promise((resolve, reject) => {
Expand Down
1 change: 1 addition & 0 deletions playground/ssr-deps/package.json
Expand Up @@ -2,6 +2,7 @@
"name": "test-ssr-deps",
"private": true,
"version": "0.0.0",
"type": "module",
"scripts": {
"dev": "node server",
"serve": "cross-env NODE_ENV=production node server",
Expand Down