fix: support for firefox/safari worker output (fix #2029)

[NotWoods]

Description

By default, Vite targets all browsers with ES module support. That criteria includes browsers that support modules but not module workers, notably Firefox & Safari.

This feature adds an alternate worker bundle output that can be used as a fallback if {type:'module'} workers aren't available. The bundle is equivalent to what would be outputted for inline scripts, but is a separate chunk so the data doesn't need to be loaded unless necessary. This double bundling is done only on build, not on serve.

Additional context

Feature detection for module workers is based on this WHATWG issue.

This is a bug described in #2029, which was closed as I didn't have a reproduction. The new test case shows an example where imports can end up in the final worker build.

Would love support on reducing code duplication. It would be nice to extract the feature detection. Some of the plugin code is copied from fileToBuiltUrl.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

---

Fixes #2029, fixes #2504

[Shinigami92]

@NotWoods One of the tests is failing, could you please have a look 🙂

[milomg]

Also see #2494, which attempts to do the same thing.

Question: why is this nice to have and not minor bug?

[Shinigami92]

@modderme123 I triage around 10-20 issues/PRs a day and try to find the right labels
I read add support for ... and that sounded like a new wanted feature instead of a bug-fix

But I think I can bump this up to p3 🙂

---

But keep in mind, that this doens't directly mean that this is the next PR that will be reviewed or merged. We have very many other PRs and slightly limited people in the vite team 🙂

---

And don't mind the review needed label. We just want to slightly get rid of this label and use native GitHub filter tools.

[antfu]

Could you resolve the conflicts, and update the base branch? Thanks

[milomg]

when updating, I believe the conflict is adding await before bundle.close (I missed this when updating my own patches)

[antfu]

Tested out locally with my Monaco setup. Both this PR and #2494 emit this warning

The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten

The dist works for Chrome, but not FireFix nor Safari.

[milomg]

Monaco is already bundled for the browser, this is doing double bundling which is why there is this at the top level scope (in other words, this is probably expected behavior). I believe to get monaco working you also need to override vite's chunk handling like this. Are there errors logged in the console?

Edit: I think I remember another error was fixed in my pr by switching from output: es to output: iife

[Jarrku]

Hello, would like to chime in that this PR causes some issues during build on Github Actions for me.

I've set up a repro case in: https://github.com/Jarrku/vite-webworker-issue

Once I try to import more than 2 web worker files, the action throws an error during build. I'm not sure if this is a config error I need to adjust on GA, but I figured I'd share this here.

I also noted that using the rollup build along with @vitejs/plugin-legacy causes errors during build. This was easily fixed/ignored? by adding an early return in the transformIndexHtml function of the plugin.

Relevant files:

• Where the imports happen
• Patch package file for vite@2.3.0
• Patch package file for @vitejs/plugin-legacy@1.4.0

[milomg]

Ah interesting, I was struggling to test this with rollup as the final bundler because of #1927. It seems you may have uncovered an error that is silently being ignored in that case.

As a workaround: I wonder if setting minify to esbuild helps with your config?

Also: I can't seem to reproduce this locally... Maybe that is because it relies on something being multicore? Do you have an easy way to reproduce this on a local machine?

[Jarrku]

As a workaround: I wonder if setting minify to esbuild helps with your config?

Im not sure what you mean by this?

Also: I can't seem to reproduce this locally... Maybe that is because it relies on something being multicore? Do you have an easy way to reproduce this on a local machine?

Indeed forgot to explicitly mention, but this only happens on Github Actions, I can build locally without problems as well

[milomg]

Im not sure what you mean by this?

https://vitejs.dev/config/#build-minify

[Jarrku]

Using esbuild as minifier does solve my repro case: https://github.com/Jarrku/vite-webworker-issue/actions/runs/851487161

So that does resolve it for non-legacy users (for my minor repro)

[NotWoods]

I've had the same minify issue occur for GH Actions (I didn't realize this PR was the cause). However, I'm not sure how to resolve the issue as I'm just making a call to rollup.

[milomg]

Maybe vite or rollup has some global understanding across multiple executions of bundle completion, and it attempts to terminate other bundles when that happens?

[Jarrku]

I've had the same minify issue occur for GH Actions (I didn't realize this PR was the cause). However, I'm not sure how to resolve the issue as I'm just making a call to rollup.

Don't think it's this specific change that is causing this. I assume the same would happen for the previously inline compiled bundles. However, with this PR, compilation will happen for every webworker, making the issue alot more noticable.

[milomg]

Aha, I may have found the bug with certain assets being wiped by the bundling!

Because we are not reinitializing asset plugin but passing the same config object (https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/index.ts#L55), when the new rollup instance runs it wipes the hashmap containing all of the assets here https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/asset.ts#L40 (probably a simple check would fix this)

There is probably a similar bug preventing inline workers from outputting anything, the ideal fix would be to recreate the plugins list instead of just copying it directly, such as something like this (which resolves the esbuild issue from above I think):

const bundle = await rollup.rollup({
    input: cleanUrl(id),
    plugins: await resolvePlugins({ ...config }, [], [], [])
});

[antfu]

@NotWoods could you resolve the conflicts? Thanks!

[milomg]

In case it helps, I've resolved the conflicts in #2494 and fixed the issue with terser minification not working.

[antfu]

After the discussion with the team:

• We could like to avoid bundling twice for workers
• Currently we don't see much value of using esm in workers, and iife build works for both browsers
• In the future, we might consider add ?worker&module query to bundle ESM workers.

As the result, we might take #2494's approach instead. Thanks for your contributions.