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 Rust workers pipeline #167

Closed
wants to merge 5 commits into from
Closed

Add Rust workers pipeline #167

wants to merge 5 commits into from

Conversation

joshyrobot
Copy link

First iteration of Rust workers! A few notes:

  • Compiled with a wasm-bindgen target of either no-modules or web, depending on how the module_workers config is set (browser interest listed here)
  • A wrapper is placed at /worker.js (so only one at a time for now), and can then be started via new Worker("/worker.js", { type: "classic"/"module" }). This might pose issues with different a public_url?
  • Worker wrapper cannot be indefinitely cached (no hash in filename), but it's also very small so that may not matter
  • One potential option for wrapper naming is /workers/{binary}.js, but I'm unsure how duplicates should be handled
  • This is almost entirely just copy-pasted from the rust_app pipeline, not too sure where to put the common code
  • I haven't resolved any of the TODOs related to this pipeline yet

Checklist

  • Updated CHANGELOG.md describing pertinent changes.
  • Updated README.md with pertinent info (may not always apply).

@joshyrobot
Copy link
Author

joshyrobot commented Apr 12, 2021

I've thought a bit more about the issues raised in #46:

  • Main thread binaries may want to "fork", so rust and rust_worker should be combined, and multiple must be allowed
  • Binaries that run on page load will have data-main (implied if only 1 or 0 linked). Probably unique on the page to match current behavior
  • Binaries will be able to choose their bindgen target with data-no-modules. In a sense this means "I need to run in web workers on all browsers", but not necessarily "I will only run in web workers"
  • bindgen output scripts will be tweaked to run automatically (simple one line append)
  • Either A. expose a method to get hashed script names at runtime. Preferably with the possibility of getting all assets in the future. E.g., <link rel=css id=blah ...> and Trunk.getAssetURL("blah")
  • Or B. create an unhashed script that just imports the hashed one. I feel like this locks us out of the generally more powerful option of runtime resolving

EDIT: I forgot importScripts is crucial to no-modules working, which is only available in workers. So for now, having separate pipelines is likely the better solution. We still need to deduplicate code, though, so any feedback on how to do that would be greatly appreciated.

@thedodd
Copy link
Member

thedodd commented Apr 13, 2021

@joshyrobot love it! I will give the code an 👁️ right now, but I still need to review the discussions we've had over in #46. Overall, I'm stoked that you are tackling this. Thanks for taking the time to do so.

One thing I would say before I even got to the initial code review: don't worry about code duplication at this point. While you are doing exploratory work like this, it is way better to duplicate, iterate, find the best patterns, and then when we have something we like, we can do some final deduplication/refactoring.

Edit: So, the wasm-opt code can definitely be very easily factored out into its own module. Probably not its own "pipeline" at this point, but at a minimum we can just make it a top-level module under src/ and then invoke its functionality as needed from the various other pipelines.

@thedodd
Copy link
Member

thedodd commented Apr 13, 2021

@siku2 I know that you had quite a lot of interest in doing some stuff related to Workers. Just wanted to ping you in case you had thoughts to share on this.

@thedodd
Copy link
Member

thedodd commented May 19, 2021

@joshyrobot once you believe this is ready for review, mind marking the PR as ready for review, and then ping me? Happy to start a review on it then.

@joshyrobot
Copy link
Author

Yeah, sorry, I've just been mighty distracted lately. I'll try to pick this back up this week.

@simonsan
Copy link

simonsan commented Nov 2, 2021

Any updates on this? @joshyrobot seemed to be last active in June :/

@kristoff3r kristoff3r mentioned this pull request Dec 14, 2021
4 tasks
@joshyrobot joshyrobot closed this Mar 1, 2022
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.

3 participants