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 wasm_bindgen::script_url #3032

Closed
wants to merge 6 commits into from

Conversation

lukaslihotzki
Copy link
Contributor

Rust code needs the current script URL to locate files relative to the script, and to run Rust code with working imports in workers and worklets. An intrinsic is the clean solution to provide the script URL.

The existing #[wasm_bindgen(js_namespace = import, js_name = meta)] approach currently used in wasm-audio-worklet only works because wasm-bindgen translates this to import.meta. let o = import; let m = o.meta; would break. Additionally, this approach works on ES module targets only. document.currentScript is not accessible at all over wasm_bindgen. That's why other projects use artificially generated stack trace to extract the script URL. That's unnecessary effort, especially because wasm_bindgen already needs the script URL to locate the WASM module on most targets. Therefore, wasm_bindgen just needs to pass the path to user code.

Only the bundler target has no obviously correct implementation. I chose __webpack_public_path__, which is webpack-specific. However, it should be more useful than bail! or the file://-URL import.meta.url is replaced with during compilation.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

When omit_default_module_path (not exactly sure what this is) is true, the else arm is missing setting script_src: https://github.com/lukaslihotzki/wasm-bindgen/blob/27df7a9edd2da5135a4edf8cc17f17c7d533dd14/crates/cli-support/src/js/mod.rs#L716.

I fixed it like this:

if let OutputMode::NoModules { .. } = self.config.mode {
    "\
        if (typeof document === 'undefined') {
            script_src = location.href;
        } else {
            script_src = document.currentScript.src;
        }"
    .to_string()
} else {
    String::from("")
}

EDIT:
I tried this out further and found out that this doesn't really work at all. document.currentScript apparently is only available during the processing of the JS file, so putting it in a function call doesn't actually work. I moved this now up to the declaration of script_src, which I confirmed does work correctly.

I made a PR for this issue here: lukaslihotzki#1.

Copy link
Collaborator

@Liamolucko Liamolucko left a comment

Choose a reason for hiding this comment

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

I don't really think that this should be a public API, since any use of it will be tied to a particular target. I think it's better off rolled up as an implementation detail of #3034. I've left some comments anyway, though, since I think this code will probably get reused there.

| OutputMode::Deno
| OutputMode::Node {
experimental_modules: true,
} => format!("import.meta.url"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to use format! here, you should use .to_owned() instead.

experimental_modules: false,
} => format!("__filename"),
OutputMode::NoModules { .. } => format!("script_src"),
OutputMode::Bundler { .. } => format!("__webpack_public_path__"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

After a skim of the docs, I don't this returns the path of the current file; I think it returns the path at which the root of the webpack project is hosted. So, this won't work.

Webpack does seem to have some support for worker entrypoints, though: https://webpack.js.org/guides/web-workers/.

@daxpedda
Copy link
Collaborator

daxpedda commented Sep 2, 2022

I don't really think that this should be a public API, since any use of it will be tied to a particular target. I think it's better off rolled up as an implementation detail of #3034. I've left some comments anyway, though, since I think this code will probably get reused there.

I tried to properly understand dependent module support (#3019), but I think I lack too much context.

Just to make sure that our use-case is covered: we are simply trying to create shared memory workers using the same WASM file, to do that we need the path to the JS shim created by wasm-bindgen, all alternative solutions are terrible workarounds or come with their own limitations. @Liamolucko Is this use-case actually covered by #3019? I can definitely not see it in #3034.

@lukaslihotzki
Copy link
Contributor Author

The use case of shared memory workers is already covered by the existing code in #3034. To create shared memory workers, you cannot just use the script_url, because you need a custom JS module run by the worker to receive the WASM module and WASM memory (SharedArrayBuffer) in onmessage. Then, this custom module uses the wasm_bindgen main module to call init (with module and memory as arguments) and the worker entry point function.

How to create a worker and handle onmessage is shown in the wasm-bindgen-futures change. How to receive the module and the memory to init a WASM worker is shown in the wasm-audio-worklet example.

If #3034 has a chance to get upstreamed, I can also add a complete example with #3034 WASM workers. Previously, I did not get any feedback about #3034.

@daxpedda
Copy link
Collaborator

daxpedda commented Sep 2, 2022

The use case of shared memory workers is already covered by the existing code in #3034.

I'm aware of all the details necessary you pointed out. The part that I don't get is how does #3034 solve receiving the JS module, not the one that handles receiving the WASM module and memory, but the JS shim created by wasm-bindgen. That's the part that I can't seem to find.

@lukaslihotzki
Copy link
Contributor Author

The commit "Handle $wbg_main imports" adds a replacement of imports from $wbg_main to imports from the wasm-bindgen JS shim. You can see its use in https://github.com/rustwasm/wasm-bindgen/pull/3034/files#diff-1b7e3352a103f85c5662fd0cbba6127ab3464b67ee47e64a3b0d4d9aa34bd5e4R4.

@daxpedda
Copy link
Collaborator

daxpedda commented Sep 2, 2022

Ah, I see. It's a bit magic to me how this works but that's off-topic here.
Thanks!

@daxpedda
Copy link
Collaborator

@Liamolucko could we reconsider adding this API, as #3034 is a big change and probably very far away and the current workarounds are pretty bad?

#3169 makes this PR even smaller, @lukaslihotzki if you could rebase?

@ranile
Copy link
Collaborator

ranile commented Jan 17, 2023

I think instead of increasing wasm-bindgen public API and adding another intrinsic, this can be implemented using already exposed API, so perhaps this should be part of gloo? inline_js snippet that exposes a function which returns the script path sounds like a good solution to me. I'd happy to accept a PR adding it to gloo-utils

@daxpedda
Copy link
Collaborator

Unfortunately this isn't possible, as far as I know.
import.meta.url can be used externally, but location.href/document.currentScript.src cannot, because it is only evaluated during processing of the script file.

This is the same problem to why #3169 was necessary.

@Liamolucko
Copy link
Collaborator

@Liamolucko could we reconsider adding this API, as #3034 is a big change and probably very far away and the current workarounds are pretty bad?

Yeah, I've warmed to this now. In combination with #3168, it should be possible to create target-agnostic things that use this, which was my main problem here.

However, access to the script URL is practically useless outside of the web and no-modules targets right now, because the rest of the targets don't expose an init function and can't be used for multithreading. So, I think that this should probably just support those two for now, returning an error if it's called on one of the other targets. Proper shims can be added for other targets in later PRs.

Also, I think shim_url might be a better name for this.

@daxpedda daxpedda mentioned this pull request Jan 19, 2023
@daxpedda
Copy link
Collaborator

I made a new PR and addressed your comments in #3247 @Liamolucko.

@daxpedda
Copy link
Collaborator

Closing in favor of #3247.

@daxpedda daxpedda closed this May 11, 2023
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.

4 participants