Skip to content

Commit

Permalink
fix: avoid optimizing non-optimizable external deps (#8860)
Browse files Browse the repository at this point in the history
  • Loading branch information
bholmesdev authored Jun 29, 2022
1 parent ef749ed commit cd8d63b
Show file tree
Hide file tree
Showing 12 changed files with 78 additions and 21 deletions.
2 changes: 1 addition & 1 deletion packages/vite/src/node/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export const DEFAULT_CONFIG_FILES = [

export const JS_TYPES_RE = /\.(?:j|t)sx?$|\.mjs$/

export const OPTIMIZABLE_ENTRY_RE = /\.(?:m?js|ts)$/
export const OPTIMIZABLE_ENTRY_RE = /\.(?:(m|c)?js|ts)$/

export const SPECIAL_QUERY_RE = /[\?&](?:worker|sharedworker|raw|url)\b/

Expand Down
17 changes: 12 additions & 5 deletions packages/vite/src/node/optimizer/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
emptyDir,
flattenId,
getHash,
isOptimizable,
lookupFile,
normalizeId,
normalizePath,
Expand Down Expand Up @@ -644,20 +645,26 @@ export async function findKnownImports(
async function addManuallyIncludedOptimizeDeps(
deps: Record<string, string>,
config: ResolvedConfig,
extra?: string[],
extra: string[] = [],
filter?: (id: string) => boolean
): Promise<void> {
const include = [...(config.optimizeDeps?.include ?? []), ...(extra ?? [])]
if (include) {
const optimizeDepsInclude = config.optimizeDeps?.include ?? []
if (optimizeDepsInclude.length || extra.length) {
const resolve = config.createResolver({ asSrc: false, scan: true })
for (const id of include) {
for (const id of [...optimizeDepsInclude, ...extra]) {
// normalize 'foo >bar` as 'foo > bar' to prevent same id being added
// and for pretty printing
const normalizedId = normalizeId(id)
if (!deps[normalizedId] && filter?.(normalizedId) !== false) {
const entry = await resolve(id)
if (entry) {
deps[normalizedId] = entry
if (isOptimizable(entry, config.optimizeDeps)) {
deps[normalizedId] = entry
} else if (optimizeDepsInclude.includes(id)) {
config.logger.warn(
`Cannot optimize included dependency: ${colors.cyan(id)}`
)
}
} else {
throw new Error(
`Failed to resolve force included dependency: ${colors.cyan(id)}`
Expand Down
20 changes: 8 additions & 12 deletions packages/vite/src/node/optimizer/scan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,14 @@ import type { Loader, OnLoadResult, Plugin } from 'esbuild'
import { build, transform } from 'esbuild'
import colors from 'picocolors'
import type { ResolvedConfig } from '..'
import {
JS_TYPES_RE,
KNOWN_ASSET_TYPES,
OPTIMIZABLE_ENTRY_RE,
SPECIAL_QUERY_RE
} from '../constants'
import { JS_TYPES_RE, KNOWN_ASSET_TYPES, SPECIAL_QUERY_RE } from '../constants'
import {
cleanUrl,
createDebugger,
dataUrlRE,
externalRE,
isObject,
isOptimizable,
moduleListContains,
multilineCommentsRE,
normalizePath,
Expand Down Expand Up @@ -189,10 +185,6 @@ function esbuildScanPlugin(
'@vite/env'
]

const isOptimizable = (id: string) =>
OPTIMIZABLE_ENTRY_RE.test(id) ||
!!config.optimizeDeps.extensions?.some((ext) => id.endsWith(ext))

const externalUnlessEntry = ({ path }: { path: string }) => ({
path,
external: !entries.includes(path)
Expand Down Expand Up @@ -235,7 +227,11 @@ function esbuildScanPlugin(
// It is possible for the scanner to scan html types in node_modules.
// If we can optimize this html type, skip it so it's handled by the
// bare import resolve, and recorded as optimization dep.
if (resolved.includes('node_modules') && isOptimizable(resolved)) return
if (
resolved.includes('node_modules') &&
isOptimizable(resolved, config.optimizeDeps)
)
return
return {
path: resolved,
namespace: 'html'
Expand Down Expand Up @@ -382,7 +378,7 @@ function esbuildScanPlugin(
}
if (resolved.includes('node_modules') || include?.includes(id)) {
// dependency or forced included, externalize and stop crawling
if (isOptimizable(resolved)) {
if (isOptimizable(resolved, config.optimizeDeps)) {
depImports[id] = resolved
}
return externalUnlessEntry({ path: id })
Expand Down
11 changes: 11 additions & 0 deletions packages/vite/src/node/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
DEFAULT_EXTENSIONS,
ENV_PUBLIC_PATH,
FS_PREFIX,
OPTIMIZABLE_ENTRY_RE,
VALID_ID_PREFIX,
wildcardHosts
} from './constants'
Expand Down Expand Up @@ -91,6 +92,16 @@ export function moduleListContains(
return moduleList?.some((m) => m === id || id.startsWith(m + '/'))
}

export function isOptimizable(
id: string,
optimizeDepsConfig: ResolvedConfig['optimizeDeps']
): boolean {
return (
OPTIMIZABLE_ENTRY_RE.test(id) ||
(optimizeDepsConfig.extensions?.some((ext) => id.endsWith(ext)) ?? false)
)
}

export const bareImportRE = /^[\w@](?!.*:\/\/)/
export const deepImportRE = /^([^@][^/]*)\/|^(@[^/]+\/[^/]+)\//

Expand Down
4 changes: 4 additions & 0 deletions playground/optimize-deps/non-optimizable-include/index.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
@font-face {
font-family: 'Not Real Sans';
src: url('./i-throw-if-you-optimize-this-file.woff') format('woff');
}
9 changes: 9 additions & 0 deletions playground/optimize-deps/non-optimizable-include/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "non-optimizable-include",
"private": true,
"type": "module",
"version": "0.0.0",
"exports": {
".": "./index.css"
}
}
7 changes: 6 additions & 1 deletion playground/optimize-deps/vite.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@ module.exports = {
},

optimizeDeps: {
include: ['dep-linked-include', 'nested-exclude > nested-include'],
include: [
'dep-linked-include',
'nested-exclude > nested-include',
// will throw if optimized (should log warning instead)
'non-optimizable-include'
],
exclude: ['nested-exclude'],
esbuildOptions: {
plugins: [
Expand Down
4 changes: 4 additions & 0 deletions playground/ssr-deps/no-external-css/index.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
@font-face {
font-family: 'Not Real Sans';
src: url('./i-throw-if-you-optimize-this-file.woff') format('woff');
}
9 changes: 9 additions & 0 deletions playground/ssr-deps/no-external-css/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "no-external-css",
"private": true,
"type": "module",
"version": "0.0.0",
"exports": {
".": "./index.css"
}
}
3 changes: 2 additions & 1 deletion playground/ssr-deps/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
"read-file-content": "file:./read-file-content",
"require-absolute": "file:./require-absolute",
"ts-transpiled-exports": "file:./ts-transpiled-exports",
"no-external-cjs": "file:./no-external-cjs"
"no-external-cjs": "file:./no-external-cjs",
"no-external-css": "file:./no-external-css"
},
"devDependencies": {
"cross-env": "^7.0.3",
Expand Down
2 changes: 1 addition & 1 deletion playground/ssr-deps/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export async function createServer(root = process.cwd(), hmrPort) {
},
appType: 'custom',
ssr: {
noExternal: ['no-external-cjs']
noExternal: ['no-external-cjs', 'no-external-css']
}
})
// use vite's connect instance as middleware
Expand Down
11 changes: 11 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit cd8d63b

Please sign in to comment.