fix(terser): support pure functions for nth_identifier

[wmertens]

Description

Fixes #17409

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[yyx990803]

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.

[yyx990803]

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

[wmertens]

😓 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

[yyx990803]

Looking at terser option types, it seems mangle.properties can also contain nth functions: https://github.com/vitejs/vite/blob/main/packages/vite/src/types/terser.d.ts#L118

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?

[sapphi-red]

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

	shouldUseFake(_sassPath, _data, options) {
	// functions and importer is a function and is not serializable
	// in that case, fallback to running in main thread
	return !!(
	(options.functions && Object.keys(options.functions).length > 0) ||
	(options.importer &&
	(!Array.isArray(options.importer) || options.importer.length > 0))
	)
	},

vite/packages/vite/src/node/plugins/css.ts

Lines 2425 to 2429 in b240a83

	shouldUseFake(_lessPath, _content, options) {
	// plugins are a function and is not serializable
	// in that case, fallback to running in main thread
	return options.plugins?.length > 0
	},

vite/packages/vite/src/node/plugins/css.ts

Lines 2541 to 2549 in b240a83

	{
	shouldUseFake(_stylusPath, _content, _root, options) {
	// define can include functions and those are not serializable
	// in that case, fallback to running in main thread
	return !!(
	options.define &&
	Object.values(options.define).some((d) => typeof d === 'function')
	)
	},

For async functions, parentFunctions is used. It calls the function on the main thread from the worker by serializing the input/output.

vite/packages/vite/src/node/plugins/css.ts

Line 2166 in b240a83

parentFunctions: { internalImporter },

vite/packages/vite/src/node/plugins/css.ts

Line 2424 in b240a83

parentFunctions: { viteLessResolve },

I guess we can call sync functions on the main thread from the worker without making it async by using Atomics.wait if we want to. But I'm not sure how the performance will be.

[wmertens]

Ok I added a test and moved the toFunction into the worker, but unfortunately I can't make it work.

I don't understand how parentFunctions is supposed to work; also, do I understand correctly that when shouldUseFake is true, it runs the entire worker on the main thread?

[wmertens]

Another option would be to augment the terser options with reproducibleNames which would then give terser a function that generates a-zA-Z etc, because that's all I'm interested in, reproducible builds.

Would that be accepted?

[sapphi-red]

Ok I added a test and moved the toFunction into the worker, but unfortunately I can't make it work.

The test is failing because an unrelated test relies on the file structure. It's fine to update the snapshot by pnpm run test-unit -u.

I don't understand how parentFunctions is supposed to work

When you pass a function to parentFunctions, it will make it possible to call that function inside the worker. It currently requires the function to be async, so it won't work in this case.

do I understand correctly that when shouldUseFake is true, it runs the entire worker on the main thread?

Yes.

Another option would be to augment the terser options with reproducibleNames which would then give terser a function that generates a-zA-Z etc, because that's all I'm interested in, reproducible builds.

If terser isn't deterministic, it's a bug in terser (terser/terser#139 (comment)). Would you explain what that option does?