Skip to content

Remove faulty location.href fallback and disable --split-linked-modules by default #3279

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

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

daxpedda
Copy link
Collaborator

@daxpedda daxpedda commented Feb 2, 2023

This removes the faulty fallback to location.href on the no-modules target.

The problem is that if you are in a worker and using importScripts(), location.href can never return the path to the imported script, the shim, which is what we want, instead location.href will return the path to the script that starts the worker, which is not useful in this context and in fact will break any path created for linked modules.

Additionally it disables --split-linked-modules by default when using wasm-bindgen-cli-support, which was probably an oversight. In return I added a new environment variable WASM_BINDGEN_SPLIT_LINKED_MODULES for wasm-bindgen-test-runner to enable it for whomever requires it.

Fixes #3277.

@lukaslihotzki
Copy link
Contributor

lukaslihotzki commented Feb 2, 2023

I'm sorry for the trouble my PR caused.

Can script_src simply be set to a relative path instead? Like ./filename.js (with the filename wasm-bindgen uses). Then, link_to! would also just return relative paths. (Relative paths break web pages using the history API to change the current URL, but this isn't applicable to workers.)

Edit: This may not work if relative paths are still resolved to the original URL and not the importScripts-URL. However, can we use this: https://github.com/chemicstry/wasm_thread/blob/main/src/script_path.js

@daxpedda daxpedda force-pushed the no-modules-linked-modules branch from eef8075 to 8765faf Compare February 2, 2023 12:19
@Liamolucko
Copy link
Collaborator

Can script_src simply be set to a relative path instead? Like ./filename.js (with the filename wasm-bindgen uses). Then, link_to! would also just return relative paths. (Relative paths break web pages using the history API to change the current URL, but this isn't applicable to workers.)

No, because relative paths get resolved by fetch, new Worker etc. relative to location.href, not to the path of the current file. So, it runs into the the same problems as setting script_src to location.href.

@lukaslihotzki
Copy link
Contributor

Maybe I should not have hidden this in an edit, but have you seen my suggestion to use https://github.com/chemicstry/wasm_thread/blob/main/src/script_path.js?

@Liamolucko Liamolucko merged commit 3a939c4 into rustwasm:main Feb 2, 2023
@daxpedda
Copy link
Collaborator Author

daxpedda commented Feb 2, 2023

Edit: This may not work if relative paths are still resolved to the original URL and not the importScripts-URL. However, can we use this: https://github.com/chemicstry/wasm_thread/blob/main/src/script_path.js

Just tried it and it does work indeed, but I don't think wasm-bindgen should use this, it isn't exactly stable or standardized, to say the least, it's a pretty hacky workaround.

@lukaslihotzki
Copy link
Contributor

I looked it up and Error.stack is indeed undefined by the ES standard.

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.

Broken linked modules in no-modules target if shim is not parsed in document
3 participants