fix: broken worker urls in 3rd party modules (fix #10838)

[Danielku15]

Description

fixes #10838

As commented in the issues and we can see in the diff the main change is to introduce an additional "fallback file search" for worker files.

3rd party dependencies (from node_modules) will have their bundle output in the vite cache folder.

Situation Before: If this dependency references a worker using the ?module syntax, Vite derives a wrong file path for the worker which doesn't exist. Later the URL for the worker points to a non-existent file which is never generated. It is treated as optimized dependency.

Situation After: We check if the file importing the worker is an optimized dependency, and if yes, we try to resolve the worker file relative to the source of the optimized dependency. This way we jump back into node_modules for the worker file and the normal flow activates.

Checklist:

• Prepare main change working with vite dev
• Prepare a stub for the unit test
• Manually test the fix
• Get automatic tests running
• Manually ensure the fix works also with vite build output.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[hi-ogawa]

I haven't looked into the original reproduction and the test case here, but would adding optimizeDeps.exclude: ["@vitejs/test-dep-with-worker"] help for these situations?

When node_modules contains a code which requires Vite specific features (such as ?url), I expected it wouldn't work if it optimized, so I normally put it in optimizeDeps.exclude when the package is intended to be consumed through a vite plugin.

[Danielku15]

@hi-ogawa I have't tried this but from the issue discussion it might have worked for some people. But the problem with this approach is: The library authors don't control the config of the consumer projects. Library authors need to document then for each framework how to integrate and configure the exceptions. Depending on the Frontend Framework used it gets more complex as the vite.config might be hidden from the user. To avoid this complexity it would be good to support this scenario properly in Vite. But I agree that it might be a valid workaround until it is fixed.

e.g. Angular requires you to move to a custom builder to specify such things:

Exposing vite configuration does not align with the Angular CLI design goals.
To add additional Vite plugins please use a custom builder.
Originally posted by @alan-agius4 in angular/angular-cli#26859 (comment)

[hi-ogawa]

@Danielku15 Yeah, it's certainly unintuitive (and fair to say it's a bug) when something breaks after the code is moved to node_modules. Thanks for looking into the fix. I was merely suggesting a common workaround since it wasn't mentioned in the original issue.

Btw, it looks like import.meta.url is not working with deps optimizer in some cases. I wonder how "planned" fix there relates to the usage with ?worker.

• new URL(foo, import.meta.url) doesn't work when dependency was optimized #8427 (comment)

[Danielku15]

@Danielku15 Yeah, it's certainly unintuitive (and fair to say it's a bug) when something breaks after the code is moved to node_modules. Thanks for looking into the fix. I was merely suggesting a common workaround since it wasn't mentioned in the original issue.

Btw, it looks like import.meta.url is not working with deps optimizer in some cases. I wonder how "planned" fix there relates to the usage with ?worker.

• new URL(foo, import.meta.url) doesn't work when dependency was optimized #8427 (comment)

Very good point. The bug description looks very similar to the worker problem. I haven't used these features yet but assuming that that the assetImportMetaUrl is the relevant plugin for #8427 it might suffer from the exact same problem like the worker. Extending the code here might fix the bug:

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

Line 104 in 102c2fd

file = tryFsResolve(file, fsResolveOptions) ?? file

Hence it would make sense to put the new tryOptimizedDepResolve into a common place and share it. But I'd have to dig a bit deeper into this to understand the other plugin behavior and flow.

[Danielku15]

@hi-ogawa I moved the new resolve helper function to a shared place and use it now in the worker and assets import meta url module. According to the tests I could find as a reference in #8427 the output is now as expected with this change.

[rubenfiszel]

Hi, I'd like to use this patch. Thank you for making it. Any recommendation on how to use it before it gets upstreamed (if ever)

[Danielku15]

@hi-ogawa Trying to revive this PR. Is there something missing to bring this in? There seem to be quite some interest on the 2 issues it would solve.

[hi-ogawa]

There was a PR #13501 to solve the issue from import.meta.url side, so the approach seems a bit different. @sapphi-red Can you take a look at this PR? 🙏

[rubenfiszel]

any update on this?

[Danielku15]

Rebased the changes, should again be up-to-date and ready for merge 😁

[sapphi-red]

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:

1. Load the sourcemap in optimizedDepsPlugin's load hook and call getCombinedSourcemap in assetImportMetaUrlPlugin and use that to map to the original file
   • the downside of this is that when other plugins modify the sourcemap incorrectly, the generated code will be incorrect
2. Save a modified optimized file when optimizing so that assetImportMetaUrlPlugin can resolve it correctly (like I tried to do in feat(optimizer): support import.meta.url #13501)
   • the downside of this is that the optimize logic becomes more complicated
3. Modify the optimized file in optimizedDepsPlugin's load hook so that assetImportMetaUrlPlugin can resolve it correctly
   • the downside of this is that there will be a overhead of running the transformation every time the server starts

[Danielku15]

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.

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:

• According to the rollup docs the ids we get into the transform hook are referring to a single module which is transformed.
• In 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 like D:/Dev/myproject/src/node_modules/.vite/deps/@coderline_alphatab.js?v=9f8947bd
  • These IDs originate from the dependencies loaded before bundling.
    • addOptimizedDepInfo is called with these IDs from via this section
  • In dev-time the invidiual files of the dependencies are seemingly not bundled yet but served individually via .vite/deps/ urls.
• In build mode I get the real file path D:/Dev/myproject/src/vite-react/node_modules/@coderline/alphatab/dist/alphaTab.mjs
  • Looking at the output there is afterwards only a "index.js" bundle with all 3rd party modules and the app code bundled.
• I generally assume any bundling/splitting should already be handled by Vite and Rollup correctly as things should be correct for any main application files.
  • For main application files:
    • If files are not bundled, the modules are resolved via tryFsResolve correctly and from there we can find the worker/asset with the relative paths.
    • If files are bundled/chunked, Vite needs to generate certain files (optimized deps area?). But I'm not sure how in this case the existing code would already work.
  • For 3rd party files:
    • If they are not bundled, we newly as a fallback try go back into node_modules and try to resolve the asset/worker relatively to there. This is what this PR is trying to achieve.
    • If they are bundled/chunked we'd need to handle things more complex. Similarly to the main application file bundling I'm not sure how this would look like.

My questions:

• Trying to improve things step-by-step: Can we somehow detect the 1:1 and allow this path. and 1:n/n:1 cases and for now show a warning of the 1:n/n:1 cases not being supported.
• How would we reach the 1:n/n:1 path? In none of the frameworks or tests I was doing (dev/build) I managed to hit a path where bundling/chunking happened before this transform path was executed.
  • For the main file importing the worker/asset: Is there a stage where the 3rd party modules are bundled/split before being transformed and where is the related information available?
  • Is the optimizedDepInfoFromFile misleading and we need to loop through all matches as 'candidates' for valid paths? (not just pick the first one)
• The existing code also mainly handles 1:1 scenarios, I wonder where the 1:n/n:1 logic is placed (other plugins doing additional rewrites?).

[sapphi-red]

Can you assist spinning up a failing test showing your exact concern?

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."

I'm not so deep into how splitting/merging might happen into Vite ...

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 rewrite import.meta.url (evanw/esbuild#1492).

[Danielku15]

Awesome, thanks. I'll look into getting this test green.

[Danielku15]

@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:

1. During bundling any relative URLs in the format of new URL('<relative path>', import.meta.url) have to be adjusted to be correct according to the bundle. (similar to your change).
   • We don't know what the plugins want to do with this URL, we should ensure that the URLs point to the correct path so that the bundle really represents a "semantic merge" not a "copy merge" of the files. Even if no plugin (assets/worker) would be active, the URLs should be correct after bundling.
   • It should not be the duty of every plugin to extract and correct the URLs. Whether bundled or not, the plugins should be able to focus on resolving paths to assets/workers relative to the given OptimizedDepInfo.src
2. The changes of this PR would likely still be needed after (1). There is still quite a difference between optimized and unoptimized/physical deps as optimized deps need to still resolve assets relative to their origin.

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.

[sapphi-red]

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 use OptimizedDepInfo.src and I imagine the code will be quite different.

[Danielku15]

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

export { imageUrl } from './dir/path.js'
export const imageUrlRoot = new URL('./asset.png', import.meta.url).href;

playground/assets/dep-with-asset/dir/path.js

export const imageUrl = new URL('../asset.png, import.meta.url).href;

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:

• This way we follow more a "relative URL" instead of a "relative file path" approach.
• The bundler doesn't work in a way to serve the specific plugins for assets and workers. It is the concern of the asset & worker plugins that the URLs point to a physical file in the package and we need to adjust the paths so that the physical files are copied and served.
• It feels less invasive. If the package would ship a bundle itself, it would look also like this.

Con:

• The physical paths are not correct for the worker and asset plugin. They (and other plugins) need to resolve the source of the dependency bundle if the URL points to a physical file (this PR).

/playground/assets/node_modules/.vite/deps/@vitejs_test-dep-with-asset.js?v=1cdbd667

// ../../node_modules/.pnpm/@vitejs+test-dep-with-asset@file+playground+assets+dep-with-asset/node_modules/@vitejs/test-dep-with-asset/dir/path.js
var imageUrl = new URL("./asset.png", import.meta.url).href;

// ../../node_modules/.pnpm/@vitejs+test-dep-with-asset@file+playground+assets+dep-with-asset/node_modules/@vitejs/test-dep-with-asset/index.js
var imageUrlRoot = new URL("./asset.png", import.meta.url).href;
export {
  imageUrl,
  imageUrlRoot
};
//# sourceMappingURL=@vitejs_test-dep-with-asset.js.map

After Bundling (my understanding of your approach)

The URLs are adjusted to relative (or absolute?) file paths on disk.

Pro:

• URLs will point to the correct file paths after bundling (relative to the bundle) allowing easy resolving of paths in worker and asset plugins.

Con:

• Another plugins MUST pickup these URLs and ensure they are further transformed and served. Otherwise URLs might attempt to reach beyond the root path pointing to a path which might not be served new URL('../../../../../node_modules/@depscope/dep/subpath/test.html', import.meta.url).href where import.meta.url=http://localhost/app.js
• If 3rd party plugins want to do a custom URL logic not based on file paths they have to "undo" the bundler path logic.

Code:
/playground/assets/node_modules/.vite/deps/@vitejs_test-dep-with-asset.js?v=1cdbd667

// ../../node_modules/.pnpm/@vitejs+test-dep-with-asset@file+playground+assets+dep-with-asset/node_modules/@vitejs/test-dep-with-asset/dir/path.js
var imageUrl = new URL("../../../../../node_modules/.pnpm/@vitejs+test-dep-with-asset@file+playground+assets+dep-with-asset/node_modules/@vitejs/test-dep-with-asset/asset.png", import.meta.url).href;

// ../../node_modules/.pnpm/@vitejs+test-dep-with-asset@file+playground+assets+dep-with-asset/node_modules/@vitejs/test-dep-with-asset/index.js
var imageUrlRoot = new Url("../../../../../node_modules/.pnpm/@vitejs+test-dep-with-asset@file+playground+assets+dep-with-asset/node_modules/@vitejs/test-dep-with-asset/asset.png", import.meta.url).href;
export {
  imageUrl,
  imageUrlRoot
};
//# sourceMappingURL=@vitejs_test-dep-with-asset.js.map

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.

[sapphi-red]

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.