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

Nested namespaces #951

Open
michiel-de-muynck opened this issue Oct 10, 2018 · 9 comments
Open

Nested namespaces #951

michiel-de-muynck opened this issue Oct 10, 2018 · 9 comments

Comments

@michiel-de-muynck
Copy link

Currently, js_namespace only accepts a Rust identifier (a syn::Ident). Identifiers in JavaScript allow many more characters than Rust does, but that's not my point. It also means that you can't specify that a function or class (for constructors) belongs to a nested namespace (i.e. foo.bar.baz).

WebIDL namespaces have been discussed in issue #253 and js_namespace was implemented in #678, but the focus in these threads was on WebIDL, and "the webidl grammar does not allow nested namespaces". But do we only care about WebIDL or do we want to make wasm_bindgen usable for generating bindings to general JavaScript code? I think the latter, right?

Well in JavaScript packages, namespaces are not incredibly common and nested namespaces are definitely quite rare, but they do happen. JavaScript doesn't really have namespaces as a built-in concept, but people still make namespaces out of regular objects.

An example of a JavaScript project using nested namespaces (I admit I had to search for it, it's definitely not as common as I thought) is the game engine Phaser. When you want to crate a physics entity in Phaser, you call new Phaser.Physics.Box2D.Body(...). As far as I can tell, it's impossible to write an idiomatic wasm_bindgen binding for that new Phaser.Physics.Box2D.Body(...) call.

I see 2 possible ways to fix that:

  1. We could allow strings in the js_namespace = ... attribute, and then parse that string to see if it contains dots. Then we could treat those dot-separated identifiers as nested namespaces, and make both constructors and static methods operate on the right JS object.

  2. We could allow some form of modules/namespaces inside wasm_bindgen blocks. These would represent both Rust modules (in the generated Rust bindings) and JS namespaces (in the generated wasm/JS glue code). If we do this, I think it may be slightly more ergonomic to write wasm_bindgen glue code for JS code with (potentially nested) namespaces, but I also think this would be a lot of work.

What do you think? Given how uncommon nested namespaces are, and given that namespaces are not a JavaScript language construct but rather a way in which some people use JS objects, should we even fix this? If so, which of these 2 fixes seems like the way to go? Or should we do something else entirely? I think I could implement the first fix myself, I don't think I can do the 2nd.

@michiel-de-muynck
Copy link
Author

michiel-de-muynck commented Oct 10, 2018

I started digging into the code to see what it would take to actually do the first fix I mentioned, and I got pretty confused by several things. These points are not all 100% on topic, but I don't know where else to mention them (I don't feel like they're worth creating separate issues for):

  • Apparently, from this bit of code, js_namespace has additional behavior that I was not aware of. If a function the_function has attribute #[wasm_bindgen(js_namespace = X)], and the X in there corresponds to the Rust identifier of another imported JS type (js_name and js_class are not considered) that was imported in the same #[wasm_bindgen] block, then the generated binding is a static method on the X type (i.e. you have to call it as X::the_function()). In all other cases, the generated binding is a free function (i.e. you have to call it as just the_function()).

    I have several issues with this:

    • Why are js_name and js_class not considered? If X is an identifier that isn't valid in Rust (let's take for example jQuery's $), then you're stuck. Writing js_namespace = X imports X in JavaScript, but X isn't valid in JavaScript, it needs to be $. But js_namespace = "$", on top of not being valid in wasm-bindgen today (see the top post), wouldn't detect that "$" corresponds to the imported type X, so you can't call X::the_function() in Rust.

    • Why does this only happen if X was defined in the same #[wasm_bindgen] block? I know it's for technical reasons (I believe wasm-bindgen can't know whether or not X is defined in a different #[wasm_bindgen] block), but nothing else in wasm-bindgen works like this. I thought that the reason that we require users to specify js_class (and js_namespace) over and over on each of the methods, constructors, getters, setters, static functions, etc of an imported type, rather than specify js_name once, on the type, was so that methods/constructors/etc can be added to types from different #[wasm_bindgen] blocks. But why then do we require X to be in the same block only in this case of js_namespace (or rather, have its behavior change depending on whether it's in the same block).

    • As far as I know, none of this behavior is documented anywhere. No that's not true, there is a small mention on this page of the guide, which is in Chapter 9: Contributing to wasm-bindgen, Section 9.2: Internal design. Not exactly the first place I'd look as a user of wasm-bindgen. But this behavior definitely seems relevant to a user of wasm-bindgen.

    • With this behavior, the only difference between js_namespace = X and static_method_of = X, where X is an imported type representing a JS class in the same #[wasm_bindgen] block, seems to be that in the generated JS glue code, for the static method case this is bound (using JS's bind()) to the appropriate class (as it should be for a static method), but with js_namespace, this is not bound. This makes sense, and I get that we may want to support doing both, but it's a very subtle difference, and

      • The docs get it wrong. The docs on static_method_of say that "This is similar to the js_namespace attribute, but the usage from within Rust is different since the method also becomes a static method of the imported type." No, that's not the difference. With js_namespace, the method also becomes a static method of the imported type, sometimes.

      • The tests get it wrong. This static method gets a js_namespace binding here, not a static_method_of binding. With these bindings, the static method will get called with the wrong this parameter.

      So if both the docs and the tests get it wrong, how can we expect the users to get it right?

  • In general, the docs (and it seems the implementation too) are very confusing about whether an attribute expects a Rust identifier/type or a JavaScript one. Ok, most of the time these are the same, but they don't have to be. And creating bindings for cases where they're not is a mess. Another example, in getter = X and setter = X, X needs to be a Rust identifier again (an Ident). Shouldn't that also allow a string?

I'm wondering if it's not better to just deprecate both js_namespace and js_class and keep only js_name as an identifier or string (that may contain dots), and then when a method/constructor/getter/etc, is added to an imported class we just look up whether or not that class has a redefined js_name. Yes, that would make it so that you could only add methods/constructors/etc to a class if it's defined inside the same #[wasm_bindgen] block, but is that actually a limitation? Are there use cases where you actually want to add methods to a type imported elsewhere?

@alexcrichton
Copy link
Contributor

Thanks for the report! In the short term I think it's fine to allow strings in js_namespace which happen to contain dots, and it just ends up showing up in the generated bindings with dots as well.

As for the design of these attributes, they were all added in a pretty ad-hoc fashion over time as need for them arose. I hope you're not too offended by the current state!

For documentation, improvements are always welcome! Sending a PR is all that's necessary :)

Long-term it seems like a good idea to consolidate these attributes indeed! For now though we'd prefer to avoid breaking changes to #[wasm_bindgen]

@michiel-de-muynck
Copy link
Author

No I'm certainly not "offended" by the current state of the code. I'm just trying to understand how the current code works, and trying to get an idea of how it's supposed to work, and from there I'm trying to find a reasonable proposal for how it should be changed to make it better (in this case to support nested namespaces).

But I hope you're also not offended when I say that it's been a pretty frustrating experience. Trying to understand how the current code works or is supposed to work took quite a bit of time because, as you mention yourself, the current code is pretty "ad hoc" and not super consistent. And trying to find out how to improve it has been even more frustrating because, on top of there not being a very consistent base to build on, backwards compatibility gets in the way of things as well.

As an example: even after your previous post I still don't know how to change js_namespace's behavior. Ok, accepting strings with dots in them, that's easy. But then ast::Import::js_namespace becomes a String instead of an Ident and this line no longer compiles, which belongs to the bit of code responsible for js_namespace's weird/inconsistent behavior that I explained in my previous post. So I have to do something with that bit of code. Do I make it keep doing exactly what it did previously, i.e., see if the X in js_namespace = X matches any of the Rust identifiers of the types imported in this same #[wasm_bindgen] block (and not the js_name of those imported types)? Backwards compatibility would say yes: that's what it did so keep doing that. But it's definitely wrong for it to do that, if we compare anything we really should be comparing the JS name, not the Rust name of the imported type, because js_namespace should also be the JS name of the namespace, not the Rust name. But should this bit of code even exist to begin with? Should we even be (sometimes) generating these sort-of static methods (with no this binding)? Again, backwards compatibility says yes, but as I explained in my previous post there are several arguments for no: it's weird, subtle, and inconsistent with the other wasm-bindgen attributes. But it's certainly "less wrong" than the comparing Rust identifiers instead of JS identifiers thing.

See how that's frustrating? Every step of the way I'm second guessing myself, asking myself if a bit of code is "wrong enough" to be considered a bug to be fixed or whether backwards compatibility trumps doing the right thing. If wasm-bindgen had a very clear-cut and consistent way of doing everything, then that would make this more easy to decide, but with everything being mostly "ad-hoc" solutions that are mostly consistent (consistent enough for js_namespace's behavior to stand out as being inconsistent from the rest), then it's very hard to decide and I'm spending a lot of time getting not much done.

@piaoger
Copy link

piaoger commented Oct 16, 2018

In the short term I think it's fine to allow strings in js_namespace which happen to contain dots, and it just ends up showing up in the generated bindings with dots as well.

I like this quick solution, or using "::" to align with rust/c++ style namespace. I really tried to use dots for nest namespace before reading wasm-bindgen source and this issue :)

@alexcrichton
Copy link
Contributor

@migi sounds like the last remaining question is what to do about the attach-to-a-rust-struct vs generating a free function? If that's the case I'd recommmend just ignoring the attach-to-a-rust-struct logic whenever a string is used and then we can file an issue to clean it up when the next major version is made of wasm-bindge.

Also keep in mind that this is an open source project! That means that it can get pretty old as well as gain a lot of contributors. We don't all have the time to make sure that everything is always 100% orthogonal to everything else and completely rationalizes with all historical decisions. We do our best and issues/PRs are the best way to keep making progress!

@OddCoincidence
Copy link

I'm working on creating bindings for WebExtensions (see rustwasm/gloo#21) and nested namespaces would be really useful, e.g. for browser.alarms.

@Pauan
Copy link
Contributor

Pauan commented Mar 28, 2019

@OddCoincidence In the meantime you can use this workaround:

#[wasm_bindgen]
extern "C" {
    type Alarms;

    // Various methods...
}

#[wasm_bindgen]
extern "C" {
    type Browser;

    #[wasm_bindgen(method, getter)]
    fn alarms(this: &Browser) -> Alarms;
}

#[wasm_bindgen]
extern "C" {
    static browser: Browser;
}

And now it can be used like this: browser.alarms().foo(...)

(For older versions of wasm-bindgen you can add structural)

@OddCoincidence
Copy link

I'll do that for now then, thanks @Pauan!

@hajifkd
Copy link
Contributor

hajifkd commented Mar 29, 2020

Hello. Does

In the short term I think it's fine to allow strings in js_namespace which happen to contain dots, and it just ends up showing up in the generated bindings with dots as well.

still make sense? If so, can I make a PR for this?

alexcrichton added a commit that referenced this issue May 27, 2020
* Enable nested namespace (#951)

* Specify the namespace as array (#951)

* added an example to the document

Co-authored-by: Alex Crichton <alex@alexcrichton.com>
Perseus101 pushed a commit to Perseus101/wasm-bindgen that referenced this issue Jun 7, 2020
* Enable nested namespace (rustwasm#951)

* Specify the namespace as array (rustwasm#951)

* added an example to the document

Co-authored-by: Alex Crichton <alex@alexcrichton.com>
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

No branches or pull requests

6 participants