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

feat(worker): support dynamic worker option fields #19010

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jamsinclair
Copy link
Contributor

Description

The Problem

Vite has a hard requirement that worker options are defined statically or it throws an error. This prevents some legitimate use cases, such as when a dynamic name is used to differentiate workers. As seen in the Emscripten project (code reference)

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

This is causing some issues in trying to update Emscripten to support Vite when workers are in use.

Solution

Vite only needs to ensure that the type value is statically defined. Other fields could be dynamically computed with no effect on the Vite bundling process.

A side discussion was started in #18396 (comment) about relaxing the usage of some worker option fields to support this.

This PR hopes to solve this issue in a backwards compatible way that still ensures that Vite can have high confidence in asserting the worker type at compile time.

@jamsinclair jamsinclair changed the title feat(worker): Support dynamic worker option fields feat(worker): support dynamic worker option fields Dec 19, 2024
@jamsinclair
Copy link
Contributor Author

jamsinclair commented Dec 19, 2024

Looks like this change broke the tests in two ways 😅

  • Windows paths aren't resolving nicely with the unit tests
  • It seems to have broken the worker integration tests Solved by 7cf1551

I'll look into fixing these tomorrow.

@@ -140,7 +140,6 @@ const genWorkerName = () => 'module'
const w2 = new SharedWorker(
new URL('../url-shared-worker.js', import.meta.url),
{
/* @vite-ignore */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the comment as the options can now legitimately be used as defined.

Should we add any explicit tests in the playground or will this suffice?

Copy link
Member

Choose a reason for hiding this comment

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

It seems this was added by #7099 and looks fine to simply remove the comment 👍

@sapphi-red sapphi-red added feat: web workers p3-significant High priority enhancement (priority) labels Dec 27, 2024
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thank you! I left some minor comments.

let opts: WorkerOptions = {}
try {
opts = evalValue<WorkerOptions>(rawOpts)
} catch {
const { parseAstAsync } = await import('rollup/parseAst')
Copy link
Member

Choose a reason for hiding this comment

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

We statically import rollup/parseAst in other places so let's do it here as well.

Comment on lines +99 to +100
const optsNode = ((await parseAstAsync(`(${rawOpts})`)).body[0] as any)
.expression
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const optsNode = ((await parseAstAsync(`(${rawOpts})`)).body[0] as any)
.expression
const optsNode = (
(await parseAstAsync(`(${rawOpts})`))
.body[0] as RollupAstNode<ExpressionStatement>
).expression

Let's add some types to make it safer.

}

function extractWorkerTypeFromAst(
astNode: any,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
astNode: any,
astNode: Expression,

continue
}

if (property.type === 'Property' && property.key?.name === 'type') {
Copy link
Member

@sapphi-red sapphi-red Dec 27, 2024

Choose a reason for hiding this comment

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

Suggested change
if (property.type === 'Property' && property.key?.name === 'type') {
if (
property.type === 'Property' &&
((property.key.type === 'Identifier' && property.key.name === 'type') ||
(property.key.type === 'Literal' && property.key.value === 'type'))
) {

I think we can add support for { "type": "module" }. It would be nice to have a test for this one.

)
}

if (typeProperty?.value?.type !== 'Literal') {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (typeProperty?.value?.type !== 'Literal') {
if (typeProperty?.value.type !== 'Literal') {

I guess this ? is not necessary.

Comment on lines +8 to +29
// @Note copied from packages/vite/src/shared/utils.ts
const windowsSlashRE = /\\/g
function slash(p: string): string {
return p.replace(windowsSlashRE, '/')
}

async function createWorkerImportMetaUrlPluginTransform() {
const root = slash(path.join(import.meta.dirname, 'fixtures/worker'))
const config = await resolveConfig({ configFile: false, root }, 'serve')
const instance = workerImportMetaUrlPlugin(config)
const environment = new PartialEnvironment('client', config)

return async (code: string) => {
// @ts-expect-error transform should exist
const result = await instance.transform.call(
{ environment, parse: parseAst },
code,
path.join(root, 'foo.ts'),
)
return result?.code || result
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// @Note copied from packages/vite/src/shared/utils.ts
const windowsSlashRE = /\\/g
function slash(p: string): string {
return p.replace(windowsSlashRE, '/')
}
async function createWorkerImportMetaUrlPluginTransform() {
const root = slash(path.join(import.meta.dirname, 'fixtures/worker'))
const config = await resolveConfig({ configFile: false, root }, 'serve')
const instance = workerImportMetaUrlPlugin(config)
const environment = new PartialEnvironment('client', config)
return async (code: string) => {
// @ts-expect-error transform should exist
const result = await instance.transform.call(
{ environment, parse: parseAst },
code,
path.join(root, 'foo.ts'),
)
return result?.code || result
}
}
async function createWorkerImportMetaUrlPluginTransform() {
const config = await resolveConfig({ configFile: false }, 'serve')
const instance = workerImportMetaUrlPlugin(config)
const environment = new PartialEnvironment('client', config)
return async (code: string) => {
// @ts-expect-error transform should exist
const result = await instance.transform.call(
{ environment, parse: parseAst },
code,
'foo.ts',
)
return result?.code || result
}
}

It seems we can now simplify this and align with assetImportMetaUrl.spec.ts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: web workers p3-significant High priority enhancement (priority)
Projects
Status: Discussing
Development

Successfully merging this pull request may close these issues.

3 participants