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

fix(worker): string interpolation in dynamic worker options #19476

Merged
merged 5 commits into from
Feb 21, 2025

Conversation

christoph-pflueger
Copy link
Contributor

Description

Fixes a bug that unintentionally removes the backtick and comma at the end of a template literal in dynamic worker options.

Test failure without fix:

 FAIL  packages/vite/src/node/__tests__/plugins/workerImportMetaUrl.spec.ts > workerImportMetaUrlPlugin > with interpolated dynamic name field and static type in worker options
RollupError: Unexpected eof
 ❯ getRollupError node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/parseAst.js:396:41
 ❯ convertProgram node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/parseAst.js:1084:26
 ❯ parseAstAsync node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/parseAst.js:2070:106
 ❯ parseWorkerOptions packages/vite/src/node/plugins/workerImportMetaUrl.ts:105:8
    103|   } catch {
    104|     const optsNode = (
    105|       (await parseAstAsync(`(${rawOpts})`))
       |        ^
    106|         .body[0] as RollupAstNode<ExpressionStatement>
    107|     ).expression
 ❯ getWorkerType packages/vite/src/node/plugins/workerImportMetaUrl.ts:169:22
 ❯ Object.transform packages/vite/src/node/plugins/workerImportMetaUrl.ts:239:30
 ❯ packages/vite/src/node/__tests__/plugins/workerImportMetaUrl.spec.ts:14:20
 ❯ packages/vite/src/node/__tests__/plugins/workerImportMetaUrl.spec.ts:96:7

@christoph-pflueger
Copy link
Contributor Author

Is it even necessary to strip the trailing comma? The tests seem to pass with the comma present as well.

@sapphi-red sapphi-red added p2-edge-case Bug, but has workaround or limited in scope (priority) feat: web workers regression The issue only appears after a new release labels Feb 21, 2025
@sapphi-red
Copy link
Member

Is it even necessary to strip the trailing comma? The tests seem to pass with the comma present as well.

It was needed because we were not using parseAstAsync in the past. Now it works without stripping the trailing comma as we fallback to parseAstAsync when evalValue failed. But since evalValue is more faster, I think we can keep stripping the trailing comma and avoid fallbacking back in this case.

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.

Thanks!

@sapphi-red sapphi-red changed the title fix: string interpolation in dynamic worker options fix(worker): string interpolation in dynamic worker options Feb 21, 2025
@patak-dev patak-dev merged commit 07091a1 into vitejs:main Feb 21, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: web workers p2-edge-case Bug, but has workaround or limited in scope (priority) regression The issue only appears after a new release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants