Skip to content

Renamed wasm_bindgen package to wasm #910

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

Closed
wants to merge 4 commits into from

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Aug 26, 2021

WebAssembly seems to be a ever-growing section of the Rust ecosystem. Given that some aspects of it have already crept into the core rules (the thread on #772 shows functionality on common rules to support WASM), I think it'd be best to find a well defined home for this functionality which would encompass more than just wasm-bindgen.

Changes

  • All references to @rules_rust//wasm_bindgen have been moved to @rules_rust//wasm
  • //rust/private:transitions.bzl has been moved to //wasm/private:transitions.bzl (it's only use was for rust_wasm_bindgen)
  • Docs have been updated from rust_wasm_bindgen to rust_wasm.
  • Re-exported symbols have been added where rules were previously defined to allow for easy migrations

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this seems reasonable, but it seems like a lot of churn? I guess maybe worth doing as we're vaguely prepping for a 1.0 release, but I'm not sure it's worth the churn?

@UebelAndre
Copy link
Collaborator Author

I guess this seems reasonable, but it seems like a lot of churn? I guess maybe worth doing as we're vaguely prepping for a 1.0 release, but I'm not sure it's worth the churn?

I think it feels like a bunch churn because of the number of generated files being moved. Looking at the history of this package, there doesn't seem to be a whole lot of activity and it seems like the rust_wasm_bindgen rule only gets a functional (nearly) biannually. I'm really interested in having more support for wasm and would like to encorperate solutions like #771 into this package. This is the primary reason for the move as use of wasm does not mean use of wasm-bindgen. If you feel strongly about not doing this, what would you recommend as an alternative?

@illicitonion
Copy link
Collaborator

I'm not concerned about churn in our files - my concern is for users pulling in updates to the rules. Presumably at some point we're going to remove the backward compat shims?

rules_foreign_cc has been doing a bunch of this style of cleanups/renames/reorganisations recently, and as a user it's been quite frustrating to have to look up relatively trivial renames I've needed to apply to WORKSPACE and BUILD files across a bunch of repos when pulling in updates. While they've been properly doing semver bumps to tell me things may break, it's still some effort to fix things when they do break.

I guess one question is: When we actually have something other than bindgen in wasm, are we going to want to move all of the stuff currently in wasm to wasm/bindgen to namespace it? Or do we expect wasm to be a flat dump of everything wasm-related? It would be sad to have a further rename down the line :)

@UebelAndre
Copy link
Collaborator Author

I'm not concerned about churn in our files - my concern is for users pulling in updates to the rules. Presumably at some point we're going to remove the backward compat shims?

rules_foreign_cc has been doing a bunch of this style of cleanups/renames/reorganisations recently, and as a user it's been quite frustrating to have to look up relatively trivial renames I've needed to apply to WORKSPACE and BUILD files across a bunch of repos when pulling in updates. While they've been properly doing semver bumps to tell me things may break, it's still some effort to fix things when they do break.

I guess one question is: When we actually have something other than bindgen in wasm, are we going to want to move all of the stuff currently in wasm to wasm/bindgen to namespace it? Or do we expect wasm to be a flat dump of everything wasm-related? It would be sad to have a further rename down the line :)

Good point! I don't see a need to make a wasm/bindgen package but that might be attractive if we end up supporting another module in this directory. Perhaps it'd be worth making a wasm/defs.bzl to house common definitions? This way we can move most of the implementation into wasm/private for easier refactoring in the future. Do you think a defs.bzl file here would be enough to guard against future churn?

@illicitonion
Copy link
Collaborator

I'm not concerned about churn in our files - my concern is for users pulling in updates to the rules. Presumably at some point we're going to remove the backward compat shims?
rules_foreign_cc has been doing a bunch of this style of cleanups/renames/reorganisations recently, and as a user it's been quite frustrating to have to look up relatively trivial renames I've needed to apply to WORKSPACE and BUILD files across a bunch of repos when pulling in updates. While they've been properly doing semver bumps to tell me things may break, it's still some effort to fix things when they do break.
I guess one question is: When we actually have something other than bindgen in wasm, are we going to want to move all of the stuff currently in wasm to wasm/bindgen to namespace it? Or do we expect wasm to be a flat dump of everything wasm-related? It would be sad to have a further rename down the line :)

Good point! I don't see a need to make a wasm/bindgen package but that might be attractive if we end up supporting another module in this directory.

I presumed you had one coming - otherwise the existing wasm_bindgen seems like a pretty reasonable name for this package :)

Perhaps it'd be worth making a wasm/defs.bzl to house common definitions? This way we can move most of the implementation into wasm/private for easier refactoring in the future. Do you think a defs.bzl file here would be enough to guard against future churn?

Sounds reasonable, though I think I'd still be tempted to hold off on this until we have a second thing...

@UebelAndre
Copy link
Collaborator Author

I presumed you had one coming - otherwise the existing wasm_bindgen seems like a pretty reasonable name for this package :)

My thought was adding a wasm.bzl file or some sorts to define a #771 (which I'm envisioning is just a cdylib built behind a transition to the wasm platform). What I meant by that was specifically writing a rule based on another crate (maybe wasm-pack? who knows). I actually want to try and have something which doesn't use another crate but can definitely see some potential for adding uses of others.

Sounds reasonable, though I think I'd still be tempted to hold off on this until we have a second thing...

fair enough. I can hold off for now.

@UebelAndre UebelAndre closed this Sep 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants