-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
fix(optimizer): map relative worker paths to correct relative file location #21434
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
base: main
Are you sure you want to change the base?
Conversation
e579679 to
160562a
Compare
160562a to
aa501ea
Compare
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
|
I have updated the PR to address the recent feedback and improve the stability of the implementation:
|
|
I'm open to discussing the workerRE regex logic, as I tried to balance false positives and false negative rate versus readability and simplicity. The current regex doesn't handle these cases:
const workerRE =
/(new\s+(?:Shared)?Worker\s*\(\s*)new\s+URL\s*\(\s*(['"])((?:(?!\2).|\\.)*)\2\s*,\s*import\.meta\.url\s*\)/g |
| const assetDir = path.dirname(absolutePath) | ||
|
|
||
| if ( | ||
| environment.config.server.fs.allow && | ||
| !environment.config.server.fs.allow.includes(assetDir) | ||
| ) { | ||
| environment.config.server.fs.allow.push(assetDir) | ||
| } |
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.
I guess this part is not needed because it'll be handled by the other internal plugins.
| const workerRE = | ||
| /(new\s+(?:Shared)?Worker\s*\(\s*)new\s+URL\s*\(\s*(['"])((?:(?!\2).|\\.)*)\2\s*,\s*import\.meta\.url\s*\)/g |
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.
Improved Regex: Refined the transformation logic to target new Worker(new URL(...)) and new SharedWorker(...) constructors specifically. This prevents accidental transformation of standard assets (images, data files) that use new URL but are not workers.
We have the same issue with non-worker assets. Was there a specific reason to limit to the workers? If not, would you revert that change and add tests for non-worker assets as well?
| if (!id.includes('node_modules')) return null | ||
|
|
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.
| if (!id.includes('node_modules')) return null |
If the file is bundled by the optimizer, I think we need to resolve the path in new URL.
| let match | ||
| while ((match = workerRE.exec(code))) { | ||
| hasReplacements = true | ||
| const [fullMatch, prefix, _, url] = match |
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.
Let's skip URLs that starts with protocols:
if (isDataUrl(url)) {
continue
}and new URL with /* @vite-ignore */.
I think you can copy some code from
vite/packages/vite/src/node/plugins/assetImportMetaUrl.ts
Lines 62 to 75 in ebda8fd
| let s: MagicString | undefined | |
| const assetImportMetaUrlRE = | |
| /\bnew\s+URL\s*\(\s*('[^']+'|"[^"]+"|`[^`]+`)\s*,\s*import\.meta\.url\s*(?:,\s*)?\)/dg | |
| const cleanString = stripLiteral(code) | |
| let match: RegExpExecArray | null | |
| while ((match = assetImportMetaUrlRE.exec(cleanString))) { | |
| const [[startIndex, endIndex], [urlStart, urlEnd]] = match.indices! | |
| if (hasViteIgnoreRE.test(code.slice(startIndex, urlStart))) continue | |
| const rawUrl = code.slice(urlStart, urlEnd) | |
| if (!s) s = new MagicString(code) | |
fixes #21422
What is this PR solving?
This PR fixes an issue where Web Workers defined in dependencies using
new URL('./worker.js', import.meta.url)fail to load after dependency optimization (Rolldown).The Problem
In the new dependency optimizer (Rolldown), libraries are bundled into
.vite/deps. When a library contains a relative worker constructor, that relative path becomes invalid because the actual worker file remains in the originalnode_modulesdirectory. Rolldown's internal asset scanner attempts to resolve these paths relative to the new bundle location, leading to "File does not exist" errors and broken worker loading.The Solution
I added a
transformhook to thevite:dep-pre-bundleplugin withinrolldownDepPlugin.ts:new URLpatterns within optimized dependencies and rebases them to the correct relative paths pointing to the original source file./* @vite-ignore */and string concatenation ('' + import.meta.url) to make the expression dynamic. This effectively "blinds" Rolldown's static analysis scanner, preventing it from incorrectly attempting to bundle the absolute path as a relative asset.server.fs.allow, ensuring the dev server permits the browser to fetch the worker via the absolute path.Unit Test & Housekeeping Updates
packages/vite/src/node/__tests__/plugins/import.spec.tsto reflect minor formatting changes (tabs to spaces) triggered by the project's Prettier configuration. No logic changes were made to these core tests.pnpm-lock.yamlto the latest format to resolve workspace integrity issues encountered when adding the new playground test package.How has this been tested?
I added a new test case in the
optimize-depsplayground:@vitejs/test-dep-with-worker-reprowhich exports workers from the root and a nested directory.playground/optimize-deps/__tests__/optimize-deps.spec.tsto verify that worker messages are correctly received in the browser logs during dev mode.Summary of Changes
rolldownDepPlugin.tsnew URLand rebases to absolute paths.import.spec.tspnpm-lock.yamlplayground/optimize-deps/...Before vs After Tests
Failed Test Log (Before Fix)
Test Log (After Fix)
```bash (base) guycohen@Guys-MacBook-Air vite % pnpm vitest --config vitest.config.e2e.ts playground/optimize-deps -t "worker"DEV v4.0.17 /Users/guycohen/dev/vite
--------------------------------------------------*
Test Files 1 passed | 1 skipped (2)
Tests 1 passed | 50 skipped (51)
Start at 15:46:53
Duration 3.70s (transform 86ms, setup 720ms, import 68ms, tests 883ms, environment 0ms)
PASS Waiting for file changes...
press h to show help, press q to quit