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

Update worker PR #1

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Conversation

dnaka91
Copy link

@dnaka91 dnaka91 commented Feb 24, 2022

This updates the PR for web worker support to not create a wrapper script, but instead skip the hash for file naming and use browser features to pass the initialization script to the worker initializer.

I added samples for both vanilla code and using the gloo packages.

After all is merged into Trunk I'll add more comments and do some cleanup here and there for the examples.

@dnaka91 dnaka91 mentioned this pull request Feb 24, 2022
4 tasks
@oberien
Copy link

oberien commented Feb 24, 2022

I think the original worker.js (maybe under a different name) should still be available for projects that don't want to use blob: imports. With trunk-rs#288 the idea is to allow a Content-Security-Policy which is as strict as can be (given wasm is being executed). With the solution of this PR using blob instead of the previous worker.js, the CSP must also include script-src blob:.

If the old behaviour (worker.js) and the new behaviour (blob:...) are both supported, documentation should be added for the following gotcha: The old worker.js uses absolute paths to refer to the js and wasm scripts to be included. When blob is used on that file directly, a SyntaxError is fired as the joined URI will be blob:/worker_bg.wasm which isn't valid. As such, absolute URIs are required to load resources from within a blob: context.
I think either the old worker.js should join the absolute path onto window.location.origin to make the URIs absolute, allowing blob:-context-workers to load the worker.js with, or documentation should be added to describe both cases individually. (I'm mentioning this problem because when I tried to use gloo_worker with the rust_worker branch I ran into this exact issue and it took me several hours to understand what was going on.)

@dnaka91
Copy link
Author

dnaka91 commented Feb 24, 2022

You don't really have to use the blob: version, that's just what gloo-worker is doing behind the scenes, and the vanilla sample does the same by hand.

My goal is to not dictate devs what way to load the worker. Some might like the blob, others might like a separate file. But what is always troublesome is when the dist folder contains files that you don't need.

So I would like to give the dev the choice, either use blobs, use a separate js file or doe it in some other way.

Eventually the wrapper could be generated as an opt-in, but it's probably not asked too much to create that small file yourself and add it with the copy-file link option.

@oberien
Copy link

oberien commented Feb 24, 2022

Makes sense, I agree. You can always move out the blob into its own file and load it from there. For the toy examples, such a self-contained approach is probably easier to understand.

@kristoff3r kristoff3r merged commit a68f045 into kristoff3r:rust_worker Feb 24, 2022
@dnaka91 dnaka91 deleted the worker-fixes branch February 25, 2022 10:08
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