Skip to content
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

Add web worker support #285

Merged
merged 3 commits into from
Feb 26, 2022
Merged

Add web worker support #285

merged 3 commits into from
Feb 26, 2022

Conversation

kristoff3r
Copy link
Contributor

Fixes #46

I started work on this because it looks like #167 is stalled. I took some inspiration from it, but I ended up with a different design that solves some of the questions joshyrobot raised.

Instead of a separate rel="rust-worker" asset type, which duplicates a lot of attributes and code, I made a data-type attribute for rel="rust" which can be either main or worker. For backwards compatibility reasons I made main the default, even though it complicates the code a bit. Workers are built with wasm-bindgen --no-modules, and a small wrapper is included that loads the hashed module. The wrapper is named after the binary name if provided, otherwise the project name. I have also used this name to make the logs and file names clearer, as it got confusing fast when multiple modules were named index-{hash}.

My use case for this is for use in a yew project. I have tested it with a private yew project, but annoyingly it requires a PR to yew as well, to tell it to use the trunk generated wrapper instead of creating a new one. I'll probably create that one if some variant of this PR gets merged.

Feel free to bikeshed both the design and implementation. I suspect there might be considerations related to e.g. #203 and the issues it links that are relevant to this.

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

@yusdacra
Copy link

This PR is really nice, I am using it for my app and it works perfectly. Would love to see this merged!

@thedodd
Copy link
Member

thedodd commented Jan 14, 2022

Woot woot! cc'ing @dnaka91 on this as well, as he has been helping out quite a lot. Him and I are planning on cutting a new release soon, and we'll def prioritize getting this factored in as well!

Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really great. One item that I think we need to change for backwards compatibility, and one mini nit.

Thanks for the great work here!

site/content/assets.md Show resolved Hide resolved
src/pipelines/rust.rs Outdated Show resolved Hide resolved
src/pipelines/rust.rs Outdated Show resolved Hide resolved
@ranile
Copy link
Contributor

ranile commented Jan 14, 2022

My use case for this is for use in a yew project. I have tested it with a private yew project, but annoyingly it requires a PR to yew as well, to tell it to use the trunk generated wrapper instead of creating a new one. I'll probably create that one if some variant of this PR gets merged.

Please note that yew-agent, which is where the change should've gone is now gloo-worker. Feel free to PR against that but do keep in mind that gloo-worker, being a part of gloo toolkit, is supposed to build tool/framework agnostic.

@dnaka91
Copy link
Contributor

dnaka91 commented Jan 15, 2022

If it's not asked too much, it would be awesome to have a minimal example added to the existing examples.

This would help to:

  • Get other people starter with trunk + web workers quickly, as they can use the example as starting point.
  • Help in manual testing before release, as well as build future integration tests quickly, using the example as reference.

This allows web workers to be included in `index.html`. Since this makes the
rust-worker attribute unnecessary it has been removed.

Also adds an example showing basic usage and updates the documentation.
@kristoff3r
Copy link
Contributor Author

Thanks for the feedback, I have rebased on master, changed the 2 things mentioned and added an example. The example doesn't do anything interesting because it is annoying to communicate back with only web_sys, but it does successfully load the worker. Ideally I guess we would add support in gloo-worker and use that in the example?

@ranile
Copy link
Contributor

ranile commented Jan 19, 2022

@kristoff3r why can't we use gloo-worker as is? What's the problem that comes up? Can you file an issue explaining the problem?

@kristoff3r
Copy link
Contributor Author

kristoff3r commented Jan 19, 2022

@kristoff3r why can't we use gloo-worker as is? What's the problem that comes up? Can you file an issue explaining the problem?

It needs an option to not create a javascript wrapper in the url blob storage, as this PR already makes a javascript wrapper, and it doesn't work to have both. Maybe I'll have time in the weekend to add it and update the example, or at least create the issue.

@Wandalen
Copy link

Vote up for that :)

@dnaka91
Copy link
Contributor

dnaka91 commented Feb 24, 2022

After learning about web workers and reading a lot of the gloo-worker source code, I updated the webworker sample and added another webworker-gloo sample, that uses the gloo-worker crate.

@kristoff3r please have a look at the PR kristoff3r#1 that I just opened and adds the necessary changes to make everything work.

The main change is that, for web workers, we shouldn't add a hash to the output files as they have to be named when instantiating the Worker class in the browser (through JS or Rust). Therefore, I changed it to not add the hash for workers.

Also, I think we shouldn't create a wrapper script for the hashed files, and leave it to the developer how to instantiate the worker. You can see in the samples how to achieve this without loading an extra file. That reduces one level of indirection as well.

If you're okay with the changes, please merge them so that they are reflected here. After that we should be good to go on merging this PR 👍

@kristoff3r
Copy link
Contributor Author

@dnaka91 Thanks for doing the remaining work. I have been away from this PR for a bit due to other work, but the research I had been doing also led me to conclude that we shouldn't hash the worker. It would be nice at some point, but that requires a much more ambitious integration in the style of #9.

I have merged your changes and I think it's good to go (from my side at least).

@dnaka91
Copy link
Contributor

dnaka91 commented Feb 25, 2022

Thanks for merging @kristoff3r. Over in the other PR @oberien made a good point about the wrapper script. We agreed in the end that Trunk (for now) shouldn't do too much and leave it to the user how to actually instantiate the worker.

I used the blob feature in the samples and will merge this PR after the CI passes. But after that I'll extend that vanilla sample a bit to add a second worker that is done with a wrapper script instead. Just to show off both ways for beginners.

@dnaka91 dnaka91 dismissed thedodd’s stale review February 26, 2022 08:12

All requested changes have been done

@dnaka91 dnaka91 merged commit b989bc9 into trunk-rs:master Feb 26, 2022
@joshyrobot
Copy link

Sorry I never got around to my original PR, but it looks like this one handles it all quite well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for including Web Workers
7 participants