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 support for linked modules #3069

Merged
merged 27 commits into from
Jan 31, 2023
Merged

Conversation

lukaslihotzki
Copy link
Contributor

@lukaslihotzki lukaslihotzki commented Sep 5, 2022

Like imported modules, linked modules can be added by any crate with the wasm_bindgen::link_to! macro and the existing module, raw_module or inline_js attributes. In contrast to imported modules, no imports are added to the JS shim generated by wasm-bindgen. Instead, the macro expands to an expression of type String that contains a link. This link may be relative and is suitable for creating workers or worklets, or dynamic imports.
Internally, the macro invocation wasm_bindgen::link_to!(module = "/src/task/worker.js") expands to code like (simplified):

{
	pub static _GENERATED: [u8; …] = …; // Program in custom section
	extern "C" {
		fn __wbindgen_link_9fdd114fc6bc2308() -> String; // link function
	}
	__wbindgen_link_9fdd114fc6bc2308()
}

This link function is implemented in the JS shim:

    imports.wbg.__wbindgen_link_9fdd114fc6bc2308 = function(arg0) {
        const ret = new URL('snippets/wasm-bindgen-futures-b12f15e5e844bdeb/src/task/worker.js', import.meta.url).toString();
        …
    };

The main reason for this approach is that it works both without bundlers and with bundlers (like webpack >= 5) that detect the new URL(…, import.meta.url) syntax, bundle and minify the linked file, and change the link. Depending on the target, the correct alternative for import.meta.url is used.

This is a useful part of #3034. In contrast to #3034, importing the JS shim is not supported, so this PR can only be used for pure JS workers or worklets (not considering workarounds).

Fixes #2335 (Just create a script element or dynamically import your linked module before using JS classes from Rust)

@alexcrichton
Copy link
Contributor

Apologies but I personally do not have the time to adequately review new major features like this. Others may be able to but I will be unable to provide review or issue support for this if it lands.

@ranile ranile self-requested a review September 15, 2022 21:37
@ranile
Copy link
Collaborator

ranile commented Sep 15, 2022

Requesting my own review so I don't forget. This is something I've experimented with and can be very useful when it comes to lazy loading modules

@lukaslihotzki
Copy link
Contributor Author

I needed to rebase because of #3073. Because the link_to! macro is new, strict attribute checking can be enabled without breaking change.

Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

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

The implementation looks fine to me. It's worth noting that I'm not too familiar with this part of the code base so I could've missed something important.

A couple of things are missing here. Can you add tests and documentation for this feature?
It's not really possible to update the book1 so adding rustdoc comments (including an example) to the macro would be fine.

Footnotes

  1. I tried reaching out to @fitzgen for push access to the repo(s) but couldn't get a hold of them. If anyone else seeing this can grant me access, that would be great.

@lukaslihotzki lukaslihotzki force-pushed the linked-module branch 2 times, most recently from 287b461 to 2f3da4a Compare September 26, 2022 00:34
crates/macro/src/lib.rs Outdated Show resolved Hide resolved
crates/macro/ui-tests/link-to.stderr Outdated Show resolved Hide resolved
crates/backend/src/codegen.rs Outdated Show resolved Hide resolved
crates/backend/src/codegen.rs Outdated Show resolved Hide resolved
@lukaslihotzki lukaslihotzki force-pushed the linked-module branch 2 times, most recently from 12d6449 to 5ea1515 Compare September 26, 2022 13:48
@lukaslihotzki lukaslihotzki force-pushed the linked-module branch 2 times, most recently from d9dde32 to 3b8542b Compare January 27, 2023 02:15
@lukaslihotzki lukaslihotzki force-pushed the linked-module branch 2 times, most recently from 62a172a to da157a5 Compare January 27, 2023 03:15
@lukaslihotzki
Copy link
Contributor Author

Other bundlers are a valid concern. Luckily, it can be solved at bindgen time, because the generated JS import code can choose to return a data URI. It took me some time though to figure out embedding the content in AuxImport::LinkTo is a lot nicer than trying to pass WasmBindgenAux to the import code generator. Compared to compile time approaches, this prevents a dependency to set the feature unexpectedly. Also, this guarantees a consistent link-or-embed behavior and allows to display explanatory text if links are needed. Alternatively, links could be allowed automatically if embedding is impossible (maybe displaying a hint).

@Liamolucko, I'm sorry for the delay. Do you accept this approach? If so, I'll have to add tests with embedding probably. Is there anything else missing (I don't think so)?

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.

Good idea to put it behind a flag! I think that works out well; by default, everything 'just works', and then if you want the stricter CSP and/or lazy loading, you can manually configure your bundler to make things work and turn on the flag.

crates/cli-support/src/wit/mod.rs Outdated Show resolved Hide resolved
crates/cli/src/bin/wasm-bindgen.rs Outdated Show resolved Hide resolved
crates/cli-support/src/js/mod.rs Outdated Show resolved Hide resolved
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.

Looks good. I just have some nitpicks about the phrasing of the docs.

guide/src/reference/cli.md Outdated Show resolved Hide resolved
guide/src/reference/cli.md Outdated Show resolved Hide resolved
guide/src/reference/cli.md Outdated Show resolved Hide resolved
guide/src/reference/cli.md Outdated Show resolved Hide resolved
guide/src/reference/cli.md Outdated Show resolved Hide resolved
Co-authored-by: Liam Murphy <liampm32@gmail.com>
@lukaslihotzki
Copy link
Contributor Author

@Liamolucko, I have overlooked the other suggestion. It is committed now.

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.

Thank you for all of your work on this!

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.

Lazy loading of JS snippets
4 participants