Use --target=web for WebWorkers

[JonasAlaif]

I ran into the following error when using a web worker which uses a JS snippet (namely, using the viz_js crate in a web worker):

error: importing from `...` isn't supported with `--target no-modules`

I didn't dig into it too much, but this page seems to explain the issue. So, as this PR does, I tried to naively change to --target=web in trunk and this resolved the above error for me.

Not sure why @kristoff3r chose to use --target=no-modules in #285, maybe there is some advantage? If there is some reason for no-modules then it would at least be nice to be able to toggle this in a config.

Checklist

• Updated CHANGELOG.md describing pertinent changes.
• Updated README.md with pertinent info (may not always apply).
• Updated site content with pertinent info (may not always apply).
• Squash down commits to one or two logical commits which clearly describe the work you've done. If you don't, then Dodd will 🤓.

[ctron]

I am wondering why that is necessary. According to the wasm-bindgen docs, it should be --target=no-modules:

https://rustwasm.github.io/wasm-bindgen/examples/wasm-in-web-worker.html:

At the time of this writing, only Chrome supports modules in web workers, e.g. Firefox does not. To have compatibility across browsers, the whole example is set up without relying on ES modules as target. Therefore we have to build with --target no-modules. The full command can be found in build.sh.

I am not sure this is true anymore: https://bugzilla.mozilla.org/show_bug.cgi?id=1247687

[JonasAlaif]

Thanks for those pointers, very useful.

I actually didn't test with running the compiled code, only that trunk can compile it, my bad. It seems that the resulting code doesn't work on any of Chrome, Firefox or Safari. I get the errors Uncaught SyntaxError: Cannot use import statement outside a module (at worker.js:1:1), SyntaxError: import declarations may only appear at top level of a module, SyntaxError: Unexpected token '{'. import call expects one or two arguments. respectively. Where the first line of worker.js is:

import { graphviz_dummy_hack } from './snippets/viz-js-3dff437496ba69e3/js/viz-standalone.js';

Investigating a bit more, I think that one would need to create the worker like this to avoid the error. Looking under the "options.type parameter" support table here I think that all but Safari support "modules in web workers" (i.e. the page you link is a bit outdated indeed). So the reason that my code breaks is that I'm using yew-agent/gloo-worker which creates the web worker here with the simple new, but instead I would need to create it using this method with the type option correctly set.

Building my project without a dependency on viz_js (so that I avoid the import) still causes an error (in Firefox SyntaxError: export declarations may only appear at top level of a module due to export class IntoUnderlyingByteSource in worker.js). Therefore, I think that Worker::new_with_options(.., type={classic, module}) needs to be synchronized with --target={no-modules,web} here.

To summarize, I think that this PR should instead expose a config option to select between no-modules/web for web workers. Then, to get my code working, I should add a similar config option to gloo-worker to get it to use new_with_options.

[ctron]

@hamza1311 Would you be able to take a look too. IIRC you worked with the workers too.

[ranile]

I explained my stance on it at rustwasm/gloo#421 (comment):

Every major browser supports it so I don't see the need to keep the no-modules around. There is no user code that needs to be updated, as far as I'm aware, as all the JS is generated by wasm-bindgen and it calls WASM.

[ctron]

@hamza1311 @JonasAlaif so I guess it makes sense then to have this configurable and provide reasonable defaults. Basically what you did, but switching all default to web. Right?

[ctron]

Btw, I think it would help if you remove the changelog entry. Moving forward, this should work with conventional commits, rather than a manual changelog file.

[JonasAlaif]

Sorry that I didn't cross-link the gloo PR, I was meaning to but forgot.
Sounds good. Everything mentioned should now be implemented: let me know if I missed anything.

[JonasAlaif]

There is one more issue: data-loader-shim needs to be updated to generate the correct one for web.

[JonasAlaif]

I think this is ready to be merged now (unless anyone spots any remaining issue).

Changing the default will require everyone to either use the new data-bindgen-target flag to revert to the old behavior or switch to creating workers with new_with_options though. So definitely a breaking change and we might want to include some info to help people to update (and anyone using gloo-worker will have to revert the behavior or update to a new version with this PR)

[ctron]

@JonasAlaif before merging anything into trunk, I want to see how the situation about the governance of trunk resolves. Right now there the PR #623, which would bring back the changes from trunk-ng into trunk. Merging this PR would make merging the other even more painful. However, the other one cannot be merged since I don't have permission to merge (vs rebase) a PR.

If that helps, you can rebase this PR on trunk-ng and I will merge and release it. And then backport this into trunk (proper) once this is resolved.

[JonasAlaif]

Yep sounds reasonable, the rebased one on trunk-ng will be the one to merge after #623

[kristoff3r]

Not sure why @kristoff3r chose to use --target=no-modules in #285, maybe there is some advantage? If there is some reason for no-modules then it would at least be nice to be able to toggle this in a config.

I don't remember the details, but in general I did the minimum amount to get it to work, and I think the other targets might have had worse support back then.

[ctron]

@JonasAlaif I think it needs to be rebased after the trunk-ng merge. Other than that, I think it's good to be merged. One thing that might be an improvement could be to fully parse the target into an enum, not sure that's worth it though.

[JonasAlaif]

@ctron will do. I did try parsing it to an enum, but it was a bit awkward: one needs to implement Display to get back exactly the same string that was just parsed (also this way the value the user writes will always match the actual string passed in --target and adding new values is as simple as changing the ensure check). No strong preference either way though so I can change it to the enum if you prefer.

[ctron]

Looks good to me.

I would only have a minor request. Could you format your commit message using "conventional commits": https://www.conventionalcommits.org/en/v1.0.0/#summary

In this case I think it's just prefixing the first line with feat: , as it's adding a new feature.

[JonasAlaif]

Does that look ok? I added the "BREAKING CHANGE" thing with the changed default

[ctron]

Yes that looks fine. But I wasn't fully aware that this would be a breaking change :) … But I think it could be easily turned into a non-breaking changing by using the old default, and put in a "todo" to change the default for 0.19.0. Sorry for spotting that so late.

[ctron]

Ok, reading through the gloo issue, I've the feeling that changing the default makes sense. Breaking change or not. So hopefully it doesn't actually break anything … otherwise, sorry for doing so. But there seems an easy way back to the old setting.

[ctron]

I rebased the PR and merged it directly to main in 9a9c970

[ctron]

Ok, I reverted the default to no-modules as it actually broke stuff in the examples folder of trunk. As I don't understand the full details, I'll go back to the previous default, having the new code in place too. So it's no longer a breaking change.

I will need to take a closer look into workers. But that takes more time and I would like the get out 0.18.1.

[JonasAlaif]

@ctron no worries, the reason it's a breaking change is that wasm-bindgen will generate a "module" .js file (with e.g. import/export lines) when using --target=web while it will avoid those when using --target=no-modules. In order for a worker to successfully be spawned when its js file is a "module" one needs to create it with type=module, in Rust this means creating the worker using new_with_options.

For the Trunk examples, this would mean

• Changing this line (this shim code should also probably be replaced by instead using the builtin Trunk data-loader-shim here)
• Changing this line once this PR is merged (the version of gloo in that example is already very outdated)

[ctron]

Ok, so I think it was good not making this a breaking change.

I added an example for module based webworkers with your suggestions: #649 … at least for the plain webworker. To my understanding gloo needs to make some changes first. I updated the gloo dependencies anyway.