From 8b8d5061ea44affd3b9bed97da2e629254a6ec28 Mon Sep 17 00:00:00 2001 From: Evan You Date: Sat, 9 Jan 2021 22:53:51 -0500 Subject: [PATCH] refactor(optimizer): adjust node built-in handling - prefer resolved if resolvable - externalize to empty module when not resolved - better plugin warning handling --- docs/config/index.md | 14 +--- .../optimize-deps-linked-include/index.js | 9 +++ packages/vite/src/node/build.ts | 79 ++++++------------- packages/vite/src/node/optimizer/index.ts | 35 +++----- packages/vite/src/node/plugins/resolve.ts | 25 ++++-- 5 files changed, 59 insertions(+), 103 deletions(-) diff --git a/docs/config/index.md b/docs/config/index.md index 336e83984b31e8..6507755e824364 100644 --- a/docs/config/index.md +++ b/docs/config/index.md @@ -415,22 +415,10 @@ export default ({ command, mode }) => { ### optimizeDeps.exclude -- **Type:** `string[]` +- **Type:** `string | RegExp | (string | RegExp)[]` Dependencies to force exclude in pre-bundling. -### optimizeDeps.link - -- **Type:** `string[]` - - Dependencies to be explicitly treated as linked source in pre-bundling. Note Vite 2.0 automatically detects linked packages (deps whose resolved path is not inside `node_modules`) so this should only be needed in rare cases. - -### optimizeDeps.allowNodeBuiltins - -- **Type:** `string[]` - - A list of dependencies that imports Node built-ins, but do not actually use them in browsers. Suppresses related warnings. - ### optimizeDeps.auto - **Type:** `boolean` diff --git a/packages/playground/optimize-deps-linked-include/index.js b/packages/playground/optimize-deps-linked-include/index.js index 508a908abefa6a..a869157f53ed1b 100644 --- a/packages/playground/optimize-deps-linked-include/index.js +++ b/packages/playground/optimize-deps-linked-include/index.js @@ -8,3 +8,12 @@ export function useCount() { // test dep with css/asset imports import './test.css' + +// test importing node built-ins +import fs from 'fs' + +if (false) { + fs.readFileSync() +} else { + console.log('ok') +} diff --git a/packages/vite/src/node/build.ts b/packages/vite/src/node/build.ts index 0c953b276f7c58..d8ef6812574f4f 100644 --- a/packages/vite/src/node/build.ts +++ b/packages/vite/src/node/build.ts @@ -8,7 +8,6 @@ import Rollup, { RollupOptions, RollupWarning, WarningHandler, - WarningHandlerWithDefault, OutputOptions, RollupOutput } from 'rollup' @@ -22,7 +21,6 @@ import { copyDir, emptyDir, lookupFile } from './utils' import { manifestPlugin } from './plugins/manifest' import commonjsPlugin from '@rollup/plugin-commonjs' import dynamicImportVars from '@rollup/plugin-dynamic-import-vars' -import isBuiltin from 'isbuiltin' import { Logger } from './logger' import { TransformOptions } from 'esbuild' import { CleanCSS } from 'types/clean-css' @@ -280,12 +278,7 @@ async function doBuild( ...options.rollupOptions, plugins: config.plugins as Plugin[], onwarn(warning, warn) { - onRollupWarning( - warning, - warn, - config.optimizeDeps?.allowNodeBuiltins, - options.rollupOptions?.onwarn - ) + onRollupWarning(warning, warn, config) } }) @@ -397,59 +390,20 @@ const dynamicImportWarningIgnoreList = [ export function onRollupWarning( warning: RollupWarning, warn: WarningHandler, - allowNodeBuiltins: string[] = [], - userOnWarn?: WarningHandlerWithDefault + config: ResolvedConfig ) { - function doWarn() { - if (userOnWarn) { - userOnWarn(warning, warn) - } else { - warn(warning) - } - } - if (warning.code === 'UNRESOLVED_IMPORT') { - let message: string const id = warning.source const importer = warning.importer - - if (importer && /\?commonjs-external$/.test(importer)) { - // allow commonjs external... - warning.message = chalk.yellow(warning.message) - return doWarn() - } - - if (id && isBuiltin(id)) { - let importingDep: string | undefined - if (importer) { - const pkg = JSON.parse(lookupFile(importer, ['package.json']) || `{}`) - if (pkg.name) { - importingDep = pkg.name - } - } - if ( - importingDep && - allowNodeBuiltins.some((allowed) => importingDep!.startsWith(allowed)) - ) { - return - } - const dep = importingDep - ? `Dependency ${chalk.yellow(importingDep)}` - : `A dependency` - message = - `${dep} is attempting to import Node built-in module ${chalk.yellow( - id - )}.\n` + - `This will not work in a browser environment.\n` + - `Imported by: ${chalk.gray(importer)}` - } else { - message = - `[vite]: Rollup failed to resolve import "${warning.source}" from "${warning.importer}".\n` + - `This is most likely unintended because it can break your application at runtime.\n` + - `If you do want to externalize this module explicitly add it to\n` + - `\`build.rollupOptions.external\`` + // throw unless it's commonjs external... + if (!importer || !/\?commonjs-external$/.test(importer)) { + throw new Error( + `[vite]: Rollup failed to resolve import "${id}" from "${importer}".\n` + + `This is most likely unintended because it can break your application at runtime.\n` + + `If you do want to externalize this module explicitly add it to\n` + + `\`build.rollupOptions.external\`` + ) } - throw new Error(message) } if ( @@ -460,6 +414,17 @@ export function onRollupWarning( } if (!warningIgnoreList.includes(warning.code!)) { - doWarn() + const userOnWarn = config.build.rollupOptions?.onwarn + if (userOnWarn) { + userOnWarn(warning, warn) + } else if (warning.code === 'PLUGIN_WARNING') { + config.logger.warn( + `${chalk.bold.yellow(`[plugin:${warning.plugin}]`)} ${chalk.yellow( + warning.message + )}` + ) + } else { + warn(warning) + } } } diff --git a/packages/vite/src/node/optimizer/index.ts b/packages/vite/src/node/optimizer/index.ts index 380f88bfda4310..befeb4cca8751e 100644 --- a/packages/vite/src/node/optimizer/index.ts +++ b/packages/vite/src/node/optimizer/index.ts @@ -25,6 +25,7 @@ import aliasPlugin from '@rollup/plugin-alias' import commonjsPlugin from '@rollup/plugin-commonjs' import jsonPlugin from '@rollup/plugin-json' import { buildDefinePlugin } from '../plugins/define' +import { createFilter } from '@rollup/pluginutils' const debug = createDebugger('vite:optimize') @@ -36,21 +37,17 @@ export interface DepOptimizationOptions { /** * Do not optimize these dependencies. */ - exclude?: string[] - /** - * A list of linked dependencies that should be treated as source code. - */ - link?: string[] - /** - * A list of dependencies that imports Node built-ins, but do not actually - * use them in browsers. - */ - allowNodeBuiltins?: string[] + exclude?: string | RegExp | (string | RegExp)[] /** * Automatically run `vite optimize` on server start? * @default true */ auto?: boolean + /** + * A list of linked dependencies that should be treated as source code. + * @deprecated + */ + link?: string[] } export interface DepOptimizationMetadata { @@ -174,7 +171,7 @@ export async function optimizeDeps( input: qualified, external, onwarn(warning, warn) { - onRollupWarning(warning, warn, options.allowNodeBuiltins) + onRollupWarning(warning, warn, config) }, plugins: [ aliasPlugin({ entries: config.alias }), @@ -219,19 +216,6 @@ export async function optimizeDeps( e.message += `\n\n${chalk.cyan( path.relative(root, e.loc.file) )}\n${chalk.dim(e.frame)}` - } else if (e.message.match('Node built-in')) { - e.message += chalk.yellow( - `\n\nTip:\nMake sure your "dependencies" only include packages that you\n` + - `intend to use in the browser. If it's a Node.js package, it\n` + - `should be in "devDependencies".\n\n` + - `If you do intend to use this dependency in the browser and the\n` + - `dependency does not actually use these Node built-ins in the\n` + - `browser, you can add the dependency (not the built-in) to the\n` + - `"optimizeDeps.allowNodeBuiltins" option in vite.config.js.\n\n` + - `If that results in a runtime error, then unfortunately the\n` + - `package is not distributed in a web-friendly format. You should\n` + - `open an issue in its repo, or look for a modern alternative.` - ) } throw e } @@ -271,13 +255,14 @@ async function resolveQualifiedDeps( const pkg = JSON.parse(pkgContent) const deps = Object.keys(pkg.dependencies || {}) const linked: string[] = [] + const excludeFilter = exclude && createFilter(exclude) for (const id of deps) { if (include && include.includes(id)) { // already force included continue } - if (exclude && exclude.includes(id)) { + if (excludeFilter && excludeFilter(id)) { debug(`skipping ${id} (excluded)`) continue } diff --git a/packages/vite/src/node/plugins/resolve.ts b/packages/vite/src/node/plugins/resolve.ts index c9c8956bc05a67..0906beb9a806e5 100644 --- a/packages/vite/src/node/plugins/resolve.ts +++ b/packages/vite/src/node/plugins/resolve.ts @@ -146,14 +146,6 @@ export function resolvePlugin({ // bare package imports, perform node resolve if (bareImportRE.test(id)) { - // externalize node built-ins only when building for ssr - if (isBuild && config && config.build.ssr && isBuiltin(id)) { - return { - id, - external: true - } - } - if (asSrc && server && (res = tryOptimizedResolve(id, server))) { return res } @@ -170,6 +162,23 @@ export function resolvePlugin({ ) { return res } + + // node built-ins. + // externalize if building for SSR, otherwise redirect to empty module + if (isBuiltin(id)) { + if (isBuild && config && config.build.ssr) { + return { + id, + external: true + } + } else { + this.warn( + `externalized node built-in "${id}" to empty module. ` + + `(imported by: ${chalk.white.dim(importer)})` + ) + return browserExternalId + } + } } isDebug && debug(`[fallthrough] ${chalk.dim(id)}`)