feat: allow worker options via variable

[mame]

Description

This PR allows a variable reference in worker options of new Worker() as follows:

const myWorkerOptions = { type: 'module' }
...
new Worker(new URL(..., import.meta.url), myWorkerOptions)

Background: Emscripten 3.1.58 (or later) generates such a code. I thought about fixing it on the emscripten side, but it would be better if the vite could support this style. See also emscripten-core/emscripten#22394

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[sapphi-red]

Thanks for the PR.

Even if we add support for this case, we'll still have many unsupported cases.

const type = 'module'
new Worker(new URL(..., import.meta.url), { type })

const opts = { type: 'module' }
new Worker(new URL(..., import.meta.url), { ...opts, name: 'foo' })

import { opts } from './worker.utils'
new Worker(new URL(..., import.meta.url), opts)

If we keep adding support for these cases, both the document and the implementation will be bloated.
So I prefer to keep it in the current state.

But let's see what others think.

[patak-dev]

I also vote to keep things as is here. It is unfortunate that we have this issue with emscripten, but if it is possible to fix this there, that should be more clear and maintainable for the future. As Sapphi explained, once we open this door, we should probably support a lot more cases or it will be quite confusing.

[mame]

Thank you for your comments. If such a decision is made, I will comply.

I would like you to look at the corresponding generation code of Emscripten:

https://github.com/emscripten-core/emscripten/blob/10cb9d46cdd17e7a96de68137c9649d9a630fbc7/src/library_pthread.js#L396-L462

As you can see, workerOptions is used in 4 places. Inlining the definition in each place would be obviously never “more clear and maintainable”.

I see no need to extend this support unlimitedly. I thought that the style such as var workerOptions could be frequent in the generated code and is worth supporting in Vite. Just IMO.

[jamsinclair]

Quickly noting that in the Emscripten issue thread it looks like the maintainer is eager to solve it on the Emscripten side 👀

Also if any Vite users stumble upon this PR, I also have a simple Vite Plugin that will patch the issue with Emscripten files (Plugin Gist Link)

[jamsinclair]

ヤッホー @sapphi-red @hi-ogawa 👋 , I wanted to propose relaxing worker options to be only partially static? 🤔

The main reasoning I understand for the restriction is determining whether the worker type is classic or module. As this impacts the bundling approach. Other worker option fields are not used.

A valid use case for dynamic worker options is used in the Emscripten project. To help with debugging, custom names can be given to a worker (Code reference).

e.g.

worker = new Worker(new URL('target.js', import.meta.url), {
  'type': 'module',
  'name': 'em-pthread-' + PThread.nextWorkerID,
})

I could see other projects also wanting this functionality. To support this while also trying to balance code bloat and edge cases I wanted to propose only allowing inline worker options.

The restrictions would be:

// Worker options must be defined inline as an object
✅ new Worker(new URL(..., import.meta.url), { name: 'worker-' + someId });
❌ new Worker(new URL(..., import.meta.url), opts);

// The type field must be statically defined if used
✅ new Worker(new URL(..., import.meta.url), { name: 'worker-' + someId, type: 'module' });
✅ new Worker(new URL(..., import.meta.url), { name, type: 'module' });
❌ new Worker(new URL(..., import.meta.url), { name: 'worker-' + someId, type });

// All field names must be statically defined, no spread operator
✅ new Worker(new URL(..., import.meta.url), { name, credentials: 'include', type: 'module' });
❌ new Worker(new URL(..., import.meta.url), { type: 'module', ...opts });

A rough idea of the business logic

1. Use regex or string functions to assert that options begin and end with { }
2. Use regex or string functions to assert if type is defined and has static value
3. Return static type value or default to classic

I totally understand if this invites too much complexity in regards to documentation and potential edge cases 😅

[sapphi-red]

@jamsinclair
A dynamic name property sounds like a reasonable thing to support. Vite already supports that for ?worker. I think we can draw the line at "whether the type property is possible to determine by just looking at the option parameter" (so that spread operator is supported as long as type: 'something' exists later than that, e.g. { ...foo, type: 'module' }) if that doesn't make things much complicated.
I guess we can fallback to ast parse using (await import('rollup/parseAst')).parseAst(`(${rawOpts})`) when inside this catch clause to achieve that.

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

Lines 35 to 41 in 3400a5e

	} catch {
	throw err(
	'Vite is unable to parse the worker options as the value is not static.' +
	'To ignore this error, please use /* @vite-ignore */ in the worker options.',
	optsStartIndex,
	)
	}

[jamsinclair]

Thanks @sapphi-red. That sounds like a good approach and falling back to AST would make that check quite easy.

[sapphi-red]

Closing as #19010 is merged and the remaining parts would probably be resolved on the emscripten side.

[mame]

Great, I will give it a try later. Thank you for your great work!