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

chore: make terser an optional dependency #8049

Merged
merged 25 commits into from Jun 8, 2022
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
de7f351
chore: make terser an optional dependency
sapphi-red May 6, 2022
cc697cf
chore(plugin-legacy): add terser to peer dep
sapphi-red May 20, 2022
faac5cd
chore: downgrade terser requirements to 5.4.0
sapphi-red May 20, 2022
715ca98
Merge branch 'main' into chore/terser-optional
sapphi-red May 20, 2022
e077d7e
chore: improve terser not found message
sapphi-red May 20, 2022
ecfc295
docs: use Terser instead of terser
sapphi-red May 21, 2022
caacc4e
chore: simplify terser not found message
sapphi-red May 21, 2022
18c1d59
Merge branch 'main' into chore/terser-optional
sapphi-red May 21, 2022
9278fc2
fix: terser import
sapphi-red May 21, 2022
ec34abf
docs: use Terser instead of terser
sapphi-red May 21, 2022
0b054fc
test: set `emptyOutDir: false` for flaky tests
sapphi-red May 21, 2022
f3d8450
chore(deps): bump vitest to 0.12.9 from 0.12.4
sapphi-red May 22, 2022
27e1c18
Merge branch 'main' into chore/terser-optional
sapphi-red May 22, 2022
925fd58
chore: use require inside worker
sapphi-red May 24, 2022
593df66
Merge branch 'main' into chore/terser-optional
sapphi-red May 24, 2022
eb0cc61
Revert "chore: use require inside worker"
sapphi-red May 24, 2022
706840f
Revert "Revert "chore: use require inside worker""
sapphi-red May 24, 2022
c8d2e2d
Merge branch 'main' into chore/terser-optional
sapphi-red May 27, 2022
80de213
chore: add comment about import inside worker
sapphi-red May 28, 2022
6385a9b
chore: add comment about emptyOutDir
sapphi-red May 28, 2022
d784dff
Merge branch 'main' into chore/terser-optional
sapphi-red May 31, 2022
d449ace
chore: add return type to requireResolveFromRootWithFallback
sapphi-red May 31, 2022
b6ec807
fix: use createRequire(root)
sapphi-red May 31, 2022
8e1d22b
Revert "fix: use createRequire(root)"
sapphi-red May 31, 2022
92113ec
Merge branch 'main' into chore/terser-optional
sapphi-red Jun 8, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/config/build-options.md
Expand Up @@ -147,6 +147,12 @@ Set to `false` to disable minification, or specify the minifier to use. The defa

Note the `build.minify` option is not available when using the `'es'` format in lib mode.

Terser must be installed when it is set to `'terser'`.

```sh
npm add -D terser
bluwy marked this conversation as resolved.
Show resolved Hide resolved
```

## build.terserOptions

- **Type:** `TerserOptions`
Expand Down
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -87,7 +87,7 @@
"unbuild": "^0.7.4",
"vite": "workspace:*",
"vitepress": "1.0.0-draft.4",
"vitest": "^0.12.4",
"vitest": "^0.12.9",
"vue": "^3.2.33"
},
"simple-git-hooks": {
Expand Down
6 changes: 6 additions & 0 deletions packages/plugin-legacy/README.md
Expand Up @@ -27,6 +27,12 @@ export default {
}
```

Terser must be installed because plugin-legacy uses Terser for minification.

```sh
npm add -D terser
bluwy marked this conversation as resolved.
Show resolved Hide resolved
```

## Options

### `targets`
Expand Down
1 change: 1 addition & 0 deletions packages/plugin-legacy/package.json
Expand Up @@ -42,6 +42,7 @@
"systemjs": "^6.12.1"
},
"peerDependencies": {
"terser": "^5.4.0",
"vite": "^3.0.0-alpha"
},
"devDependencies": {
Expand Down
10 changes: 5 additions & 5 deletions packages/vite/package.json
Expand Up @@ -19,9 +19,6 @@
},
"./client": {
"types": "./client.d.ts"
},
"./terser": {
"require": "./dist/node-cjs/terser.cjs"
}
},
"files": [
Expand Down Expand Up @@ -116,7 +113,6 @@
"source-map-support": "^0.5.21",
"strip-ansi": "^6.0.1",
"strip-literal": "^0.3.0",
"terser": "^5.13.1",
"tsconfck": "^2.0.0",
"tslib": "^2.4.0",
"types": "link:./types",
Expand All @@ -126,7 +122,8 @@
"peerDependencies": {
"less": "*",
"sass": "*",
"stylus": "*"
"stylus": "*",
"terser": "^5.4.0"
},
"peerDependenciesMeta": {
"sass": {
Expand All @@ -137,6 +134,9 @@
},
"less": {
"optional": true
},
"terser": {
"optional": true
}
}
}
28 changes: 1 addition & 27 deletions packages/vite/rollup.config.ts
Expand Up @@ -123,10 +123,6 @@ function createNodePlugins(isProduction: boolean, sourceMap = true): Plugin[] {
// Shim them with eval() so rollup can skip these calls.
isProduction &&
shimDepsPlugin({
'plugins/terser.ts': {
src: `require.resolve('terser'`,
replacement: `require.resolve('vite/terser'`
},
// chokidar -> fsevents
'fsevents-handler.js': {
src: `require('fsevents')`,
Expand Down Expand Up @@ -190,27 +186,6 @@ function createNodeConfig(isProduction: boolean) {
})
}

/**
* Terser needs to be run inside a worker, so it cannot be part of the main
* bundle. We produce a separate bundle for it and shims plugin/terser.ts to
* use the production path during build.
*/
const terserConfig = defineConfig({
...sharedNodeOptions,
output: {
...sharedNodeOptions.output,
entryFileNames: `node-cjs/[name].cjs`,
exports: 'default',
format: 'cjs',
sourcemap: false
},
input: {
// eslint-disable-next-line node/no-restricted-require
terser: require.resolve('terser')
},
plugins: [nodeResolve(), commonjs()]
})

function createCjsConfig(isProduction: boolean) {
return defineConfig({
...sharedNodeOptions,
Expand Down Expand Up @@ -244,8 +219,7 @@ export default (commandLineArgs: any) => {
envConfig,
clientConfig,
createNodeConfig(isProduction),
createCjsConfig(isProduction),
...(isProduction ? [terserConfig] : [])
createCjsConfig(isProduction)
])
}

Expand Down
8 changes: 3 additions & 5 deletions packages/vite/src/node/plugins/css.ts
Expand Up @@ -42,7 +42,8 @@ import {
isRelativeBase,
normalizePath,
parseRequest,
processSrcSet
processSrcSet,
requireResolveFromRootWithFallback
} from '../utils'
import type { Logger } from '../logger'
import { addToHTMLProxyTransformResult } from './html'
Expand Down Expand Up @@ -1294,10 +1295,7 @@ function loadPreprocessor(lang: PreprocessLang, root: string): any {
return loadedPreprocessors[lang]
}
try {
// Search for the preprocessor in the root directory first, and fall back
// to the default require paths.
const fallbackPaths = _require.resolve.paths?.(lang) || []
const resolved = _require.resolve(lang, { paths: [root, ...fallbackPaths] })
const resolved = requireResolveFromRootWithFallback(root, lang)
return (loadedPreprocessors[lang] = _require(resolved))
} catch (e) {
if (e.code === 'MODULE_NOT_FOUND') {
Expand Down
44 changes: 29 additions & 15 deletions packages/vite/src/node/plugins/terser.ts
@@ -1,26 +1,39 @@
import { dirname } from 'path'
import { fileURLToPath } from 'url'
import { Worker } from 'okie'
import type { Terser } from 'types/terser'
import type { Plugin } from '../plugin'
import type { ResolvedConfig } from '..'
import { requireResolveFromRootWithFallback } from '../utils'

// TODO: use import()
const _dirname = dirname(fileURLToPath(import.meta.url))
let terserPath: string | undefined
const loadTerserPath = (root: string) => {
if (terserPath) return terserPath
try {
terserPath = requireResolveFromRootWithFallback(root, 'terser')
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Actually the one used in the CSS plugin should be changed to createRequire too

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it safe to remove resolving from root? I now found these related things: ddfcbce, #3988, #2612.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of

const resolved = require.resolve(name, [root, ...fallbackPaths]);
require(resolved)

I think it's safer to use:

const requireFromRoot = createRequire(path.resolve(root, 'index.cjs'))
requireFromRoot(name)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Wait 🤔 This problem is more complicated than I thought…

To clarify, I'm concerned about the require method used here is not "clean" (would be interfered with by require.cache, etc.).
Both require and require.resolve should be created from the root path so that we can get a fresh instance of terser, sass, etc. The point is that require should be returned from createRequire(some-file-in-the-root-dir) too.

Here comes the complexity:

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, my original suggestion is to make requireResolveFromRootWithFallback requireFromRootWithFallback (that is, put the require part into the function too).

It's a nice-to-have.

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'm leaving this for now because I tried now but I didn't make it work.

Copy link
Member

Choose a reason for hiding this comment

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

@sodatea do you think we could merge this then? And then work on your request as an enhancement? Or is this still blocking the PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the PR looks good to me. It's not a blocking change.

} catch (e) {
if (e.code === 'MODULE_NOT_FOUND') {
throw new Error(
'terser not found. Since Vite v3, terser has become an optional dependency. You need to install it.'
)
} else {
const message = new Error(`terser failed to load:\n${e.message}`)
message.stack = e.stack + '\n' + message.stack
throw message
}
}
return terserPath
}

export function terserPlugin(config: ResolvedConfig): Plugin {
const makeWorker = () =>
new Worker(
async (basedir: string, code: string, options: Terser.MinifyOptions) => {
// when vite is linked, the worker thread won't share the same resolve
// root with vite itself, so we have to pass in the basedir and resolve
// terser first.
// eslint-disable-next-line node/no-restricted-require, no-restricted-globals
const terserPath = require.resolve('terser', {
paths: [basedir]
})
// eslint-disable-next-line no-restricted-globals
return require(terserPath).minify(code, options) as Terser.MinifyOutput
async (
terserPath: string,
code: string,
options: Terser.MinifyOptions
) => {
// eslint-disable-next-line no-restricted-globals -- this function runs inside cjs
const terser = require(terserPath)
return terser.minify(code, options) as Terser.MinifyOutput
}
)

Expand Down Expand Up @@ -50,7 +63,8 @@ export function terserPlugin(config: ResolvedConfig): Plugin {
// Lazy load worker.
worker ||= makeWorker()

const res = await worker.run(_dirname, code, {
const terserPath = loadTerserPath(config.root)
const res = await worker.run(terserPath, code, {
safari10: true,
...config.build.terserOptions,
sourceMap: !!outputOptions.sourcemap,
Expand Down
12 changes: 12 additions & 0 deletions packages/vite/src/node/utils.ts
Expand Up @@ -787,6 +787,18 @@ export function getHash(text: Buffer | string): string {
return createHash('sha256').update(text).digest('hex').substring(0, 8)
}

export const requireResolveFromRootWithFallback = (
root: string,
id: string
) => {
// Search in the root directory first, and fallback to the default require paths.
const fallbackPaths = _require.resolve.paths?.(id) || []
const path = _require.resolve(id, {
paths: [root, ...fallbackPaths]
})
return path
}

// Based on node-graceful-fs

// The ISC License
Expand Down
3 changes: 2 additions & 1 deletion playground/legacy/package.json
Expand Up @@ -12,6 +12,7 @@
},
"devDependencies": {
"@vitejs/plugin-legacy": "workspace:*",
"express": "^4.18.1"
"express": "^4.18.1",
"terser": "^5.13.1"
}
}
3 changes: 2 additions & 1 deletion playground/preload/package.json
Expand Up @@ -13,6 +13,7 @@
"vue-router": "^4.0.15"
},
"devDependencies": {
"@vitejs/plugin-vue": "workspace:*"
"@vitejs/plugin-vue": "workspace:*",
"terser": "^5.13.1"
}
}
3 changes: 2 additions & 1 deletion playground/vitestSetup.ts
Expand Up @@ -182,7 +182,8 @@ export async function startDefaultServe() {
build: {
// esbuild do not minify ES lib output since that would remove pure annotations and break tree-shaking
// skip transpilation during tests to make it faster
target: 'esnext'
target: 'esnext',
emptyOutDir: false
},
customLogger: createInMemoryLogger(serverLogs)
}
Expand Down
19 changes: 12 additions & 7 deletions pnpm-lock.yaml

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