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

Implement shim_format #3168

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

daxpedda
Copy link
Collaborator

This adds a function that can determine during runtime if the shim generated by wasm-bindgen is a ES module or not.
The use-case is to determine how to load the JS shim when using workers or worklets.

Initially I tried to make the function only in WASM, by adding it into the module with walrus, but I was unable to figure out how to call that function in Rust.

See #3157.
Cc @Liamolucko.

@Liamolucko
Copy link
Collaborator

Sorry, I've taken far too long to respond to this.

I don't think it's a very good idea to implement this as a simple yes/no thing, because --target web and --target no-modules aren't the only possibilities. While it can't currently be used for multithreading, there's no reason why --target nodejs couldn't work in the future, and (as currently implemented) this API doesn't have any way to distinguish between that and --target no-modules.

I think we can do a bit better than just returning the current target, though (which is what I was initially envisioning when I said yes to this). All that actually matters is the format of the shim, which could be the same between different targets, and/or used by new targets added in the future. So, this is the API I'd propose:

// I can't think of any other formats we might add, but it's probably a good
// idea to make this `#[non_exhaustive]` just in case.
#[non_exhaustive]
pub enum ShimFormat {
    // The format used by `--target web`.
    // In future, this format should also be used by `--target deno`.
    EsModule,
    // The format used by `--target no-modules`.
    NoModules {
        // The name of the global emitted by `--target no-modules`.
        // By default this is `wasm_bindgen`, but it turns out that it can be configured with the
        // CLI, and we need to account for that.
        global_name: String,
    },
    // In future, this should be the format used by the shim of `--target nodejs`.
    // It'll be the same as `ShimFormat::EsModule`, except using a commonjs module instead of an
    // es module. (i.e., exports `init` and `initSync`.)
    // this can probably be left out initially since it doesn't actually exist yet.
    CommonJs,
}

pub fn shim_format() -> Result<ShimFormat, UnsupportedTargetError> {
    todo!()
}

pub struct UnsupportedTargetError {
    // possibly store the current target so that the error message can be something like 'error:
    // `shim_format` is not supported on the `bundler` target', but that's not very important.
}

What do you think?

@daxpedda
Copy link
Collaborator Author

daxpedda commented Jan 19, 2023

Unfortunately I don't really know anything about the other targets apart from web and no-modules, so thanks for covering all of the rest, it looks great to me.

Adding the global_name to no-modules is also amazing, that's an edge-case I didn't consider until now.

Will work on it!

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

I very quickly found out that this isn't gonna work out without some weird workarounds, I don't see a way to make an intrinsic function that returns a Result let alone an enum with variants that hold values.

My thought right now is to either split it into multiple functions, one function that returns a variant, including a variant for error, and one that returns the global_name or error.

Or just return a single String that contains both, e.g. "2foo" for ShimFormat::NoModules { global_name: "foo" } or "0bundler" for the error.

@daxpedda
Copy link
Collaborator Author

daxpedda commented Jan 19, 2023

I implemented it by splitting it into two functions, works just fine.
Currently I'm just returning an Option because I found it a bit overkill to introduce a new error type, but I'm happy to change it to a Result if that's desired.

@daxpedda daxpedda changed the title Implement shim_is_module Implement shim_format Jan 19, 2023
@daxpedda daxpedda mentioned this pull request Jan 30, 2023
@daxpedda
Copy link
Collaborator Author

daxpedda commented Mar 7, 2023

@Liamolucko friendly ping.

@Liamolucko
Copy link
Collaborator

Sorry, I’ve taken far too long to respond to this once again.

After linked modules started making progress, I started to question again whether this made sense, or whether we should just barrel forwards with the full version of linked modules. Since that hasn’t happened, I think it still makes sense to have this, but it also made me start to think about how this would make sense in tandem with various designs for linked modules.

I was planning to write a big long comment about different ways linked modules could expose the shim, including how it would fit together with this, but that was over a month ago now and so I’m basically going to give you a shortened version specifically about how different designs would affect this (and shim_url).

The first option, which is what #3034 implements, is that there’s some special syntax which expands to the shim url, and you import the shim manually. In this case, this PR is actually required to be able to write a target-agnostic library. Any linked module written this way will be inherently tied to the shim format, and so you’d need to have one linked module for each possible shim format and use shim_format to pick which to use.

The second option is that the shim is implicitly imported before the start of the user’s code. This means that it’s possible to write target-agnostic linked modules, but this PR is still useful in case people want to use any other target-specific code in their linked modules.

The third option, which is what made me question whether this API makes sense, is that instead of having a single shim shared between the main entrypoint and linked modules, link_to! allows specifying which target you want the shim to be, and a separate shim is generated for the linked module (unless it happens to be the same target, in which case the shim is reused).

In this case, there’s no such thing as ‘the shim’ that you’re getting the URL + format of, since there are potentially multiple.

But, honestly, this isn’t as big a deal as I was making it out to be in my head. I guess I was thinking of it as a damning problem, but it’s not that hard to work around - shim_url and shim_format could simply be defined to refer to the shim being used by the current thread. They wouldn’t be hugely useful, but they wouldn’t really hurt either, so I think it’d be fine for them to coexist with this design of linked modules.

So, in light of all that, I think I'm now happy to go ahead with this and #3247. The main concern that I had was how this would interact with option 3 above, but I realised as I was writing this that it's not really a problem at all. Do you have any thoughts?

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 took another look at the code and noticed some spots where the docs could be improved.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@lukaslihotzki
Copy link
Contributor

@Liamolucko, I saw your comment #3168 (comment) after I wrote #3247 (comment). So basically, I strongly prefer your third option to prevent fragmentation of usable libraries in the ecosystem, but maybe provide the shim as ES module only (#3355). This can also be applied without dependent modules with format-specific getters (see my comment for details).

@daxpedda
Copy link
Collaborator Author

I have to preface this with that I'm not really knowledgeable at all about the web frontend ecosystem outside of Wasm and the browser API, I never used any common tools, libraries or frameworks. My use case with all this is truly only supporting web workers and worklets and all the polyfills that come with it, be it Text(En|De)coder or Atomics.waitAsync().

I will do my best to try and wrap my head around all you said but I have only really experienced the perspective above.

The second option is that the shim is implicitly imported before the start of the user’s code. This means that it’s possible to write target-agnostic linked modules, but this PR is still useful in case people want to use any other target-specific code in their linked modules.

How exactly would that work/would you make that happen? Could you elaborate?

The third option, which is what made me question whether this API makes sense, is that instead of having a single shim shared between the main entrypoint and linked modules, link_to! allows specifying which target you want the shim to be, and a separate shim is generated for the linked module (unless it happens to be the same target, in which case the shim is reused).

I actually have a use case for this: using the ES module in window but not in a worker. It also sounds like the most flexible and complete solution from the ones you proposed.

So, in light of all that, I think I'm now happy to go ahead with this and #3247. The main concern that I had was how this would interact with option 3 above, but I realised as I was writing this that it's not really a problem at all.

I agree, I think in any case, these two PR's should not conflict with any ideas you proposed.


#3355 would really solve a lot of problems. I want to prevent any unexpected and opaque issues to arise for consumers of my libraries. For example snippets are supported by the web target but are silently ignored by the no-modules target. This can have bad consequences for users and is not really easy to understand from the code, or worse, code in a dependency, relying on this feature. So the idea of mixing and matching targets sounds great on paper, but I am afraid it will create more footguns. Having only one target would help a lot.

Of course this is only hypothetical, it's still needed for Firefox and we don't know exactly for how long still. It could also be that we are overseeing something and there is still another important use-case for it (not that I know of any).

Also I don't know how you do it, but maintaining this library on your own, with a bit of help from Alex Crichton and some contributors, seems like an enormous task to me. I'm afraid that tagging more and more complex features to the library will make it even harder to maintain without appropriate funding. My hope is that PRs like this with hopefully "simple core features" can prevent more complex integrations that have side effects we might have a hard time predicting.

On that topic, @lukaslihotzki, I would like to ping you on this again: #3330.

I remember there was an idea floating around in the issues of this repo, at some point, that not all features should go into wasm-bindgen, but wasm-bindgen should expose the capabilities to tag on features by external crates. Winit is having and discussing the same issue, Wgpu is already working on it.

This is probably a bit off-topic, as I actually think that a feature like the one we are discussing here should actually go into wasm-bindgen, it will be hard to expose mechanics for downstream crates to tag this on.


Sorry, I’ve taken far too long to respond to this once again.

It's alright ❤️

Co-Authored-By: Liam Murphy <liampm32@gmail.com>
@lukaslihotzki
Copy link
Contributor

The second option is that the shim is implicitly imported before the start of the user’s code. This means that it’s possible to write target-agnostic linked modules, but this PR is still useful in case people want to use any other target-specific code in their linked modules.

Target-agnostic linked modules would not work for all targets (for example, bundler and web provide different interfaces), so multiple shims are needed. As a special case, the ES module shim could probably import a no-modules shim, export, and clean everything up. This solution would be similar to target-agnostic linked modules in avoiding shim redundancy. The other way round is more difficult, because there is no standard import-like thing in no-modules and the location cannot be determined reliably. Implicit import on no-modules has the same problem, you would have to copy the complete shim to the top of each linked module.

@daxpedda daxpedda added the needs discussion Requires further discussion label May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Requires further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants