-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: main
Are you sure you want to change the base?
Implement shim_format
#3168
Conversation
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 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? |
Unfortunately I don't really know anything about the other targets apart from Adding the Will work on it! |
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 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 Or just return a single String that contains both, e.g. "2foo" for |
bdd2494
to
c09bb3b
Compare
I implemented it by splitting it into two functions, works just fine. |
@Liamolucko friendly ping. |
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 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 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, 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 - 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? |
There was a problem hiding this 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.
@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). |
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 I will do my best to try and wrap my head around all you said but I have only really experienced the perspective above.
How exactly would that work/would you make that happen? Could you elaborate?
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.
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.
It's alright ❤️ |
a148278
to
510533a
Compare
Co-Authored-By: Liam Murphy <liampm32@gmail.com>
510533a
to
3ca7308
Compare
Target-agnostic linked modules would not work for all targets (for example, |
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.