-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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: broken worker urls in 3rd party modules (fix #10838) #16418
Open
Danielku15
wants to merge
7
commits into
vitejs:main
Choose a base branch
from
Danielku15:fix/workers-in-3rd-party-modules
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+178
−11
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
a481a47
fix: broken worker urls in 3rd party modules (fix #10838)
Danielku15 8543320
test: add integration test for worker in 3rd party module
Danielku15 c3cab9a
fix: tests working in serve
Danielku15 4e135b7
test: update file count after new tests
Danielku15 6f04ff0
fix: import meta urls for assets in 3rd party dependencies
Danielku15 3cb19b9
test: Restore errors on files which cannot be resolved in deps
Danielku15 a44fb22
test: dependency with multiple files
sapphi-red File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export const imageUrl = new URL('../asset.png', import.meta.url).href |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { imageUrl } from './dir/path.js' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"name": "@vitejs/test-dep-with-asset", | ||
"private": true, | ||
"version": "1.0.0", | ||
"type": "module", | ||
"main": "index.js" | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { sharedBetweenWorkerAndMain } from './shared' | ||
|
||
self.onmessage = (e) => { | ||
self.postMessage(e.data) | ||
} | ||
|
||
sharedBetweenWorkerAndMain('', 'entry: worker') |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import { sharedBetweenWorkerAndMain } from './shared.js' | ||
|
||
export function startWorker(el) { | ||
sharedBetweenWorkerAndMain(el, 'entry: main') | ||
|
||
const worker = new Worker( | ||
new URL('./dep-worker.js?worker', import.meta.url), | ||
{ | ||
type: 'module', | ||
}, | ||
) | ||
worker.addEventListener('message', (e) => { | ||
console.log('message', e) | ||
sharedBetweenWorkerAndMain(el, e.data.msg) | ||
}) | ||
worker.postMessage({ msg: 'pong: worker' }) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"name": "@vitejs/test-dep-with-worker", | ||
"private": true, | ||
"version": "1.0.0", | ||
"type": "module", | ||
"main": "index.js" | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
export function sharedBetweenWorkerAndMain(el, text) { | ||
if ('WorkerGlobalScope' in globalThis) { | ||
self.postMessage({ msg: text }) | ||
} else if (document.querySelector(el).textContent.length > 0) { | ||
document.querySelector(el).textContent += '\n' + text | ||
} else { | ||
document.querySelector(el).textContent = text | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Sorry it took me a while to review this.
IIUC this code assumes the optimized file and the original file to be a 1-to-1 mapping. But that isn't true. For example, if a dependency has multiple files, it becomes a single file.
When multiple file gets bundled in a single file, we need sourcemaps (or something that includes that kind of information) to identify which file the line was defined in.
I can think of 3 ways to fix this:
optimizedDepsPlugin
's load hook and callgetCombinedSourcemap
inassetImportMetaUrlPlugin
and use that to map to the original fileassetImportMetaUrlPlugin
can resolve it correctly (like I tried to do in feat(optimizer): supportimport.meta.url
#13501)optimizedDepsPlugin
's load hook so thatassetImportMetaUrlPlugin
can resolve it correctlyThere 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 was thinking this a bit through and that's indeed tricky. Can you assist spinning up a failing test showing your exact concern? I'm not so deep into how splitting/merging might happen into Vite and keep mixing up things in my head on where we'd need to handle things differently.
My thoughts:
dev
mode we seem to always get URLs pointing to individual dependency files e.g. in a react app referencing my library I get something likeD:/Dev/myproject/src/node_modules/.vite/deps/@coderline_alphatab.js?v=9f8947bd
addOptimizedDepInfo
is called with these IDs from via this section.vite/deps/
urls.build
mode I get the real file pathD:/Dev/myproject/src/vite-react/node_modules/@coderline/alphatab/dist/alphaTab.mjs
tryFsResolve
correctly and from there we can find the worker/asset with the relative paths.My questions:
optimizedDepInfoFromFile
misleading and we need to loop through all matches as 'candidates' for valid paths? (not just pick the first one)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.
Sure. I pushed a commit that updates the test that shows my concern: "For example, if a dependency has multiple files, it becomes a single file."
To clarify, the splitting/merging happens by the optimizer. Vite uses esbuild's bundle feature to bundle some dependencies and pass the bundled dependencies to the plugin pipeline (the plugin pipeline is similar to rollup).
new URL(, import.meta.url)
becomes invalid after the optimization (esbuild's bundling) because the optimizer outputs the dependency into a different directory and also merges the files. Note that esbuild doesn't rewriteimport.meta.url
(evanw/esbuild#1492).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.
Awesome, thanks. I'll look into getting this test green.
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.
@sapphi-red I debugged into this part and I think I understand things better now. Seems that the
OptimizedDepInfo.src
is a bit a (historical?) design flaw.To keep the a good separation of concerns, multiple adaptions would be good:
new URL('<relative path>', import.meta.url)
have to be adjusted to be correct according to the bundle. (similar to your change).OptimizedDepInfo.src
What do you think about this solution design? This would allow us to merge this PR as part of (2) and focus about the bundling/optimization in a separate independent PR.
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 we go with 2 or 3 in #16418 (comment), then we don't need this PR's change because the plugins won't need to change the way to resolve
new URL(, import.meta.url)
. If we go with 1 in #16418 (comment), then we'll need a change like this PR but it won't useOptimizedDepInfo.src
and I imagine the code will be quite different.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.
This depends on how the transformed/corrected URL would look like whether this PR is needed or not. See this comparison:
Before Bundling
playground/assets/dep-with-asset/index.js
playground/assets/dep-with-asset/dir/path.js
After Bundling (my current thinking)
The URLs are adjusted to stay relative urls within the dependency just as-if the package would deliver the bundle itself.
Pro:
Con:
/playground/assets/node_modules/.vite/deps/@vitejs_test-dep-with-asset.js?v=1cdbd667
After Bundling (my understanding of your approach)
The URLs are adjusted to relative (or absolute?) file paths on disk.
Pro:
Con:
new URL('../../../../../node_modules/@depscope/dep/subpath/test.html', import.meta.url).href
whereimport.meta.url=http://localhost/app.js
Code:
/playground/assets/node_modules/.vite/deps/@vitejs_test-dep-with-asset.js?v=1cdbd667
For my own needs I am fine with either approach. For my needs even only this PR is enough.
I am only a bit worried about unexpected side effects of the bundler rewriting too much requiring us to revert the change again. A configuration option (magic comments / vite.config with regexp patterns) to control the behavior could reduce the risk at cost of complexity in Vite delivering correct results in all combinations.
As owners of Vite its your call for decision. I'd be happy to give either one a try to implement.
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.
We discussed this PR in today's team meeting.
We think this issue is better solved at esbuild level (or Rolldown level after we start using it) and would benefit more people. So we suggest helping esbuild supporting this feature (evanw/esbuild#2508). For example, providing a test case.
But if you would like to work on the fix on Vite side, we are open to having a fix on Vite side in the way 2 or 3 in #16418 (comment). It was mentioned in the meeting that
onEnd
might also be useful. But if the implementation is complex, we might not merge the PR. Even if the PR is merged, we would partially or fully revert the PR in the near future when we start using Rolldown in Vite for the optimizer.