Enable nested namespace (#951)

[hajifkd]

I let the macro parser accept String instead of Ident for js_namespace. The instance method of the class in a namespace is associated to the rust type which has the same name as the JS class without the namespace.

[alexcrichton]

Sorry for taking some time to get to this, but thanks for the PR!

I'm a little hesitant though to be doing this sort of parsing of the input string when the parsing isn't too too rigorous (it's just splitting on something). I think it'd be better if we could enhance the syntax of the attribute itself to avoid implicitly splitting on dots, for example providing a list of nested namespaces or something like js_namespace = ["foo", "bar", "baz"]

[hajifkd]

Hm, that makes sense. Let me try it.

[hajifkd]

I let the parser accept the namespace specification like

#[wasm_bindgen(js_namespace = ["nestedNamespace", "InnerClass"])]

or

#[wasm_bindgen(js_namespace = someNamespace)]

so that the backward compatibility is maintained.

I found that one test fails, but it fails even for the original commit.

In addition, js_namespace must be able to added to the constructor, but I think a different issue should be raised for that.

[ctaggart]

I'd like to provide Monico Editor bindings as an example taggartsoftware/ts2rs#1, but it needs nested namespace support.

[alexcrichton]

Ok I've finally gotten around to review, sorry again! I've got one small nit, but other than that can the documentation for js_namespace be updated as well?

[alexcrichton]

Out of curiosity, how come this was added?

[hajifkd]

I included this because the document of syn says that ExprArray requires the "full" feature. But, I now checked that the entire crate seems to work even without this "full" feature specification. Does it have something to do with how the features is applied for each sub-crate? Anyway I think the dependencies should be included here..

[alexcrichton]

Sounds good to me!

[hajifkd]

Sure, will do!

[alexcrichton]

Thanks again!

[hajifkd]

I started to be afraid that my PR breaks the backward compatibility, at least as long as wasm-pack is used...
The point is that since the structure of Import struct has been changed, the ABI becomes different. I mean, if a crate is dependent on another which uses previous version of the wasm-bindgen, this might cause a compile error like rustwasm/wasm-pack#853 as @ctaggart has found.

Of course, this is a kind of "composite" problem among wasm-bindgen and wasm-pack, but I think that maybe the only solution not to break the compatibility is changed js_namespace to the string with dot. @alexcrichton, how do you think?

[ctaggart]

this might cause a compile error like rustwasm/wasm-pack#853 as @ctaggart has found.

That issue is unrelated. The error was about wasm-bindgen from almost 2 years ago missing the --out-dir arg.

[hajifkd]

I now notice the rule for the version field. I did some experiments and it seems that cargo assumes even the PATCH version increment as a breaking change for 0.X.Y so that the ABI change does not matter, I guess, though I do not understand in which situation two dependencies of wasm-bindgen appears in Cargo.lock. It still seems to me that such dependencies causes a fatal bug.

[alexcrichton]

The ABI for wasm-bindgen-specific structures changes between each release is expected, this is why the CLI version must match the library version.

@hajifkd can you explain more why you think this is an issue?

[hajifkd]

@alexcrichton Thank you! Well, my concern was following. First, this code,

wasm-bindgen/crates/macro-support/src/lib.rs

Line 33 in 5ae6de5

program.try_to_tokens(&mut tokens)?;

expands wasm_bindgen macro with the certain version of the ABI. Thus, if, say, a crate, called A, were dependent of two crates, B and C, and the crate B were expanded by wasm_bindgen whose version is, for instance, 0.2.60, and the other crate were by 0.2.63, the expanded codes would include different binary code. Then, no version of wasm_bindgen_cli could process the code of A appropriately, since two different ABIs would be mixed.

However, now I think that my understanding of the version resolution procedure of cargo was wrong; the above thing hardly happens because if one specified the version of wasm_bindgen of B and C so that they exactly used the above version, cargo would say the two crates would be incompatible and stop compiling. In some extreme examples, like expanding the macro by hand, the change of ABI might cause some problem, but it is not something to be seriously cared about, I think.

But there is still one point I do not understand. In rustwasm/wasm-pack#853, two different version of wasm_bindgen, which seems incompatible, appear inCargo.lock. I think if these two version independently expanded the wasm_bindgen macro, then as I have written, no version of wasm_bindgen_cli could process the code and the issue could not be handled within wasm-pack.

[alexcrichton]

Yes Cargo should only allow one version of wasm-bindgen per crate graph. I don't understand how two entries for wasm-bindgen show up in Cargo.lock. That sounds like either a bad merge conflict or a bug in Cargo.

[Pauan]

@hajifkd Cargo guarantees that there will be only one version of a package (unless there is a major version bump).

For major version bumps, we haven't run into that problem yet (since we've been on 0.2 for a long time), but we would probably just tell users "don't mix different major versions of wasm-bindgen".

The issue with rustwasm/wasm-pack#853 is because they were specifying wasm-bindgen as a git repo, not a Cargo version, so it's unrelated.

[hajifkd]

Ah, I see. If there is a major version bump, Cargo allows for different versions to exist. I just thought opposite but now I understand it does not make sense. Thank you!