Web Workers not working out of the box with certain bundlers

theo-staizen commented 3 years ago

Stencil version:

 @stencil/core@2.0.3

I'm submitting a:

[x] bug report [] feature request

Current behavior:

I run into a couple of problems involving web workers and webpack yesterday. This was a react app and they were consuming the react version of our lib (built using @stencil/react-output-target), but I think the problems I am describing would happen for any webpack app.

The issues are due to the way stencil compiles .worker.js files and hardcodes the filename of the generated worker file and for using import.meta.url :

const workerPath = /*@__PURE__*/new URL('flags.worker-20711652.js', import.meta.url).href;

Webpack does not support import.meta.url. Workaround for now was to use cjs bundle instead of esm bundle.
Webpack and probably other bundlers, have no way to know that flags.worker-20711652.js is a file they should bundle and make available. We had to add a copy-webpack-plugin in our build to move *.worker-*.js files from node_modules/my-stencil-package/dist/cjs and into our webpack's output directory so that the URL generated by the line above, is available.

It seems a bit problematic, having to resort to these "hacks" to make this code work with bundlers. So I was wondering if there is an improvement to be made here, where stencil can use esm dynamic imports to load the contents of the file, instead of dynamically creating a url, using something like rollup-plugin-bundle-imports. That way bundlers will treat it as any other import and make sure the file is available in the bundle.

Expected behavior: Web workers should work transparently when importing the stencil bundle via webpack or other bundlers.

Steps to reproduce:

(You can also checkout this repo https://github.com/theo-staizen/stencil-workers)

Add a worker file to a stencil project and use it in a component
Build and Release a new version
Import the esm bundle in a webpack app.

Other information:

theo-staizen commented 3 years ago

@adamdbradley i noticed you recently released a couple of patches related to web workers. Can you please have a look at this issue as well? Thanks ❤️

fcroce commented 3 years ago

I too have this problem. Any update? cc @adamdbradley please? :)

romanroe commented 3 years ago

I reported a bug related to web workers a while ago #2914. It seems the web worker implementation could need some love :-(

theo-staizen commented 2 years ago

Bump this. Sorry for being annoying, but every time I see a new release (like 2.10.0 yesterday) and this still not addressed it feels bad, because the stencil way of writing WebWorkers is so clean compared to doing it manually, that it's such a shame it needs so many workarounds to get to work.

splitinfinities commented 2 years ago

Hey folks! Thank you for the patience. No timeline yet, but I tagged this to confirm the issue. One huge help would be yo make a reproduction repo for us, so we can quickly determine the problem and work out a solution. Can someone help us out by creating this?

theo-staizen commented 2 years ago

One huge help would be yo make a reproduction repo for us, so we can quickly determine the problem and work out a solution. Can someone help us out by creating this?

working on it. trying to get both webpack 4 and 5 since those are the ones I reported originally. if i have time i'll check other bundlers too (rollup, parcel, vite)

theo-staizen commented 2 years ago

@splitinfinities here you go: https://github.com/theo-staizen/stencil-workers (webpack 4 for now).

After you install deps, when you run npm run build in webpack4 you should see the following error:

To enable the workaround (as described in my original post above), uncomment the code in src/index.js and webpack.config.js

theo-staizen commented 2 years ago

Ok, I have added a webpack 5 example as well. The result was surprising here:

1. `import { defineCustomElements } from 'stencil-workers/loader'` which fails in webpack 4, works out of the box in Webpack 5.

This was surprising to me because I assumed we'd still need to use a CopyPlugin.

~I guess that the offending code, which tried to import the worker file via a call to new URL(), is only present in the CJS bundle (maybe through Babel or Typescript??)~

Webpack 5, somehow was able to determine that we are importing an asset with URL that and automatically included it to the dist output, without needing manual intervention. I am both impressed and terrified... Webpack 5 really is magic 🪄

/new URL(/ asset import / __webpack_require__(/! format.worker-51010d22.js / \"../stencil-workers/dist/esm/format.worker-51010d22.js\")

2. Using `import 'stencil-workers'` will result in a NEW error, that has to do with the default package.json that is produced when you generate the package using `npm init stencil` (which i did for this repo). Specifically:

{
  "name": "stencil-workers",
  "version": "0.0.1",
  "description": "Stencil Component Starter",
  "main": "dist/index.cjs.js",
  "module": "dist/custom-elements/index.js", // THIS BREAKS THINGS
  "es2015": "dist/esm/index.mjs",
  "es2017": "dist/esm/index.mjs",
}

That file is trying to load the 2 worker files from a dist/custom-elements/assets/ folder which is never created.

I am not sure what is the reasoning behind loading from custom-elements for "module", but "ES2015" and "ES2017" is using esm, but that seems to be wrong. My own component library is using esm and I vaguely remember having to change it from something else, for a stencil version with a breaking change.

theo-staizen commented 2 years ago

Added Rollup example to the repo (with workarounds in rollup.config.js).

1. Need to copy worker files to dist manually

Similar to Webpack 4, Rollup is not able to determine that new URL('format.worker-51010d22.js', import.meta.url) is something it needs to deal with and bundle the file. We don't get a compile error, but we do get a runtime one as the browser tries to load a file that doesn't exist in dist/.

To fix that, we need to introduce rollup-plugin-copy and use it to copy the worker files into dist/ (or dist/assets/ depending on how you import loader)

2. Dynamic imports in Rollup are weird...

When using import { defineCustomElements } from 'stencil-workers';, Rollup will squash everything in one index.js, which will work, but is not the intended behaviour (we want the loader to lazy load components as needed).

To stop that from happening, I switched to using a dynamic import for the loader, which will preserve it as its own file, but it introduces its own set of problems:

Needed to change worker copy path from dist/assets/ to dist
Needed to introduce a whole new plugin, @rollup/plugin-dynamic-import-vars (see below)

When lazy loading, need to use @rollup/plugin-dynamic-import-vars

Rollup only supports static strings for dynamic imports. The stencil ESM bundle uses template literals to load entry files, which are only supported in webpack, but not rollup (the code even has webpack "magic comments").

return import(
/* webpackInclude: /\.entry\.js$/ */
/* webpackExclude: /\.system\.entry\.js$/ */
/* webpackMode: "lazy" */
`./${bundleId}.entry.js${''}`).then((importedModule) => {
    {
        cmpModules.set(bundleId, importedModule);
    }
    return importedModule[exportName];
}, consoleError);

To fix this, we need to use @rollup/plugin-dynamic-import-vars, which will explode the template literal into a switch statement with a case for each matching file (i.e. *.entry.js)

johnjenkins commented 2 years ago

To fix this, we need to use @rollup/plugin-dynamic-import-vars, which will explode the template literal into a switch statement with a case for each matching file (i.e. *.entry.js)

(ahem - or give this PR a +1 :D - ahem)

theo-staizen commented 2 years ago

@johnjenkins once again to the rescue :O

ohmycode commented 2 years ago

I have a similar problem at a different point while loading Ionic (react) in a Meteor project. Meteor by default uses the "module" entry point in the loader package.json, which fails to bundle.

Like @theo-staizen said, removing that line fixes the issue for me.

theo-staizen commented 2 years ago

@rwaskiewicz thank you for re-opening the ticket.

I have spent the last couple of days updating my demo repo and testing all bundlers I think are relevant today. I have updated the README to document bugs + workarounds for each one: https://github.com/theo-staizen/stencil-workers

TL;DR:

/loader works ok , If the bundler can handle import.meta.url
- most bundlers require a specific plugin to parse import.meta.url and bundle the assets correctly
- vite works out of the box for build, but fails for dev mode
dist-custom-elements does not output worker files and expects them to be in the wrong path
- can sometimes be solved by manually copying worker files to dist, but not for all bundlers
Types for dist-custom-elements seem to be missing things
- e.g defineCustomElementMyComponent is not exposed in dist/components/index.d.ts

johnjenkins commented 1 year ago

just chiming in to say, I've found for vite, if you remove your stencil lib from vite's optimisation, things should work as expected - dev or otherwise. e.g.

import { defineConfig } from 'vite'

export default defineConfig({
  ...
  optimizeDeps: {
    exclude: ['YOUR_STENCIL_LIB']
  },
  ...

nasim-git commented 1 year ago

I have a problem related to How stencil bundles web workers. Every time I adjust something in the workers the bundler does not refresh the js chunk file name and that causes the old file to be loaded as it is cached in browser. As a result as long as the cache lives my changes are not reflecting.

I thought maybe it is related to how Stencil loads the worker files in the bundle: const workerPath = new URL("p-d0f82b65.js",import.meta.url).href

ionic-team / stencil