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 version specifications #161

Merged
merged 1 commit into from
Apr 26, 2018
Merged

Add support for version specifications #161

merged 1 commit into from
Apr 26, 2018

Conversation

alexcrichton
Copy link
Contributor

This commit adds a #[wasm_bindgen(version = "...")] attribute support. This
information is eventually written into a __wasm_pack_unstable section.
Currently this is a strawman for the proposal in ashleygwilliams/wasm-pack#101

@alexcrichton
Copy link
Contributor Author

@ashleygwilliams ok I've updated with our discussion this morning:

  • Imports that have a module name which starts with ./ cannot have version listed
  • All other imports must have a version listed
  • The custom section generated for wasm-pack only lists package names (in theory). The module = "..." directives may not be package names due to how imports sometimes work in JS. As a result, some postprocessing of the string specified to module = "..." is done to ensure that wasm-pack only sees package names, not erroneous names like foo/bar/baz
    • Names that don't start with @ assume that everything up to the first / character (if any) is the package name
    • Names that start with @ assume that everything up to the second / character (if any) is the package name

@Pauan
Copy link
Contributor

Pauan commented Apr 25, 2018

The example should use #[wasm_bindgen(module = "moment", version = "^2.0.0")]

Unlike Rust, npm does not default to semver (the string "2.0.0" matches the exact version 2.0.0).

So if you want to use semver semantics you need to add a ^ at the start (which then causes it to match anything >=2.0.0 and <3.0.0)

@alexcrichton
Copy link
Contributor Author

@Pauan ah indeed! That'll happen automatically here, but I can update example usage of this to recommend that. Currently wasm-bindgen doesn't interpret these strings at all, it just ferries them along

@ashleygwilliams
Copy link
Member

@alexcrichton @Pauan it's defnitely an interesting question what type of "semver defaults" we want to enforce given that npm and cargo have different ones. seems like a decision for wasm-pack to make, and i'm vaguely leaning towards keeping to the rust defaults in rust and translate those to npm one's just when we move the values to the package.json in the interest of keeping rust people in rust ecosystem defaults. would be curious what others thoughts are here and i'll probably open an issue on wasm pack about it

This commit adds a `#[wasm_bindgen(version = "...")]` attribute support. This
information is eventually written into a `__wasm_pack_unstable` section.
Currently this is a strawman for the proposal in ashleygwilliams/wasm-pack#101
@alexcrichton
Copy link
Contributor Author

Ok sounds good!

I think this is ready to go so I'm going to merge.

@alexcrichton alexcrichton merged commit 7937d02 into master Apr 26, 2018
@alexcrichton alexcrichton deleted the versions branch April 26, 2018 05:23
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Aug 6, 2018
First added in rustwasm#161 this never ended up panning out, so let's remove the
experimental suport which isn't actually used by anything today and hold off on
any other changes until an RFC happens.
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Aug 6, 2018
First added in rustwasm#161 this never ended up panning out, so let's remove the
experimental suport which isn't actually used by anything today and hold off on
any other changes until an RFC happens.
alexcrichton added a commit that referenced this pull request Aug 6, 2018
First added in #161 this never ended up panning out, so let's remove the
experimental suport which isn't actually used by anything today and hold off on
any other changes until an RFC happens.
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.

3 participants