-
-
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
fix(terser): support pure functions for nth_identifier #17589
base: main
Are you sure you want to change the base?
Changes from all commits
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,45 @@ | ||
import { resolve } from 'node:path' | ||
import { fileURLToPath } from 'node:url' | ||
import { describe, expect, test } from 'vitest' | ||
import { build } from 'vite' | ||
import type { RollupOutput } from 'rollup' | ||
|
||
const __dirname = resolve(fileURLToPath(import.meta.url), '..') | ||
|
||
describe('terser', () => { | ||
test('nth', async () => { | ||
const result = (await build({ | ||
root: resolve(__dirname, '../packages/build-project'), | ||
logLevel: 'silent', | ||
build: { | ||
write: false, | ||
minify: 'terser', | ||
terserOptions: { | ||
mangle: { | ||
nth_identifier: { | ||
get: (n) => { | ||
return 'prefix_' + n.toString() | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
plugins: [ | ||
{ | ||
name: 'test', | ||
resolveId(id) { | ||
if (id === 'entry.js') { | ||
return '\0' + id | ||
} | ||
}, | ||
load(id) { | ||
if (id === '\0entry.js') { | ||
return `const foo = 1;console.log(foo)` | ||
} | ||
}, | ||
}, | ||
], | ||
})) as RollupOutput | ||
expect(result.output[0].code).toContain('prefix_') | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -33,6 +33,12 @@ const loadTerserPath = (root: string) => { | |||||||||||||||||||||||||||||||||||||||||||||||||||
return terserPath | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
const toString = (obj: Record<string, any>, name: string) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
if (name in obj && typeof obj[name] === 'function') { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
obj[name] = obj[name].toString() | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
export function terserPlugin(config: ResolvedConfig): Plugin { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
const { maxWorkers, ...terserOptions } = config.build.terserOptions | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -47,6 +53,27 @@ export function terserPlugin(config: ResolvedConfig): Plugin { | |||||||||||||||||||||||||||||||||||||||||||||||||||
// test fails when using `import`. maybe related: https://github.com/nodejs/node/issues/43205 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
// eslint-disable-next-line no-restricted-globals -- this function runs inside cjs | ||||||||||||||||||||||||||||||||||||||||||||||||||||
const terser = require(terserPath) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
const nth: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Terser.SimpleIdentifierMangler | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Terser.WeightedIdentifierMangler | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| undefined = (options.mangle as any)?.nth_identifier | ||||||||||||||||||||||||||||||||||||||||||||||||||||
if (nth && typeof nth === 'object') { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
const toFunction = (obj: Record<string, any>, name: string) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
if (name in obj && typeof obj[name] === 'string') { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
const fn = eval(obj[name]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
if (typeof fn !== 'function') { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
throw new Error( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
`Failed to eval nth_identifier.${name}: not a function`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
obj[name] = fn | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
toFunction(nth, 'get') | ||||||||||||||||||||||||||||||||||||||||||||||||||||
toFunction(nth, 'consider') | ||||||||||||||||||||||||||||||||||||||||||||||||||||
toFunction(nth, 'sort') | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
return terser.minify(code, options) as Terser.MinifyOutput | ||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -87,12 +114,25 @@ export function terserPlugin(config: ResolvedConfig): Plugin { | |||||||||||||||||||||||||||||||||||||||||||||||||||
worker ||= makeWorker() | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
const terserPath = loadTerserPath(config.root) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
const nth = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
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. Looking at terser option types, it seems I was going to propose a more generic function serialize / revive mechanism, but it looks like these are the only two places where functions may appear in all terser options. @sapphi-red are you aware of other places where we use workers this way and may require handling functions? 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. For other places that require sync function options, it's falling back to running them on the main thread as the function itself might not be serializable. For example, the importer/plugin might be a 3rd party package and could reference a variable outside the function. vite/packages/vite/src/node/plugins/css.ts Lines 2167 to 2175 in b240a83
vite/packages/vite/src/node/plugins/css.ts Lines 2425 to 2429 in b240a83
vite/packages/vite/src/node/plugins/css.ts Lines 2541 to 2549 in b240a83
For async functions, vite/packages/vite/src/node/plugins/css.ts Line 2166 in b240a83
vite/packages/vite/src/node/plugins/css.ts Line 2424 in b240a83
I guess we can call sync functions on the main thread from the worker without making it async by using |
||||||||||||||||||||||||||||||||||||||||||||||||||||
typeof terserOptions.mangle === 'object' | ||||||||||||||||||||||||||||||||||||||||||||||||||||
? { ...terserOptions.mangle.nth_identifier } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
: undefined | ||||||||||||||||||||||||||||||||||||||||||||||||||||
if (nth) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
toString(nth, 'get') | ||||||||||||||||||||||||||||||||||||||||||||||||||||
toString(nth, 'consider') | ||||||||||||||||||||||||||||||||||||||||||||||||||||
toString(nth, 'sort') | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
const res = await worker.run(terserPath, code, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
safari10: true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
...terserOptions, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
sourceMap: !!outputOptions.sourcemap, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
module: outputOptions.format.startsWith('es'), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
toplevel: outputOptions.format === 'cjs', | ||||||||||||||||||||||||||||||||||||||||||||||||||||
mangle: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
typeof terserOptions.mangle === 'object' | ||||||||||||||||||||||||||||||||||||||||||||||||||||
? { ...terserOptions.mangle, nth_identifier: nth as any } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
: terserOptions.mangle, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
return { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
code: res.code!, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Have you tried running this code? The
Worker
is using https://github.com/sapphi-red/artichokie and it stringifies its content to run in worker threads so it cannot access variables / functions declared outside of its scope.The
toFunction
needs to be moved to inside the worker.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.
It's kinda surprising to me that the tests passed with this change - it looks like we don't have a test case covering terser minification. /cc @sapphi-red
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.
😓 sorry I didn't test it, I was hoping the CI would catch any errors. I'll add a test case for both n_th options