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

Publish the `js-sys` crate #519

Closed
fitzgen opened this Issue Jul 19, 2018 · 11 comments

Comments

Projects
None yet
4 participants
@fitzgen
Copy link
Member

fitzgen commented Jul 19, 2018

I think we have enough bindings written that it would be useful to publish now. Especially since it is a separate crate from wasm-bindgen now.

Things we need to do first:

  • Do a once over of the bindings to double check we aren't returning Number objects and all that

  • Decide on a version. Do we want to match wasm-bindgen or start at 0.1.0?

  • #[derive(Clone, Debug)] on all the extern types

  • Make sure we are always passing by-ref

  • Agree on &str vs &JsString arguments

@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Jul 19, 2018

I think we should probably reset to 0.1.0 since the whole point of a separate crate is the freedom to publish independently and not be super tied together.

@fitzgen fitzgen referenced this issue Jul 19, 2018

Closed

js-sys: Expose bindings to ALL the global JS things #275

394 of 394 tasks complete
@data-pup

This comment has been minimized.

Copy link
Member

data-pup commented Jul 19, 2018

Assuming that wouldn't have any sort of negative ergonomic effect on people using these crates, resetting to 0.1.0 makes sense to me, especially if they are separate. That said, my opinion is not strongly held by any means, and would defer to y'all's opinions on it :)

@alexcrichton

This comment has been minimized.

Copy link
Collaborator

alexcrichton commented Jul 19, 2018

Sounds good to me! (along with 0.1.0)

@alexcrichton

This comment has been minimized.

Copy link
Collaborator

alexcrichton commented Jul 20, 2018

Some things I've noticed working with this crate:

@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Jul 24, 2018

Strings are somewhat inconsistent, sometimes arguments are &str, others are JsString

I think these should always be &JsString. That way, we can let users choose if/when to make the copy from the JS heap into wasm linear memory. When host bindings are a thing that is actually happening, we can revisit to match what is fastest for them.

It's inconsistent whether arguments are by-ref or by-value

I think we should use by-ref to avoid ref-count noise.

Some methods expose all the options while others only expose some.

I don't think this needs to block publishing 0.1.0, since there are just so many optional arguments to various functions and methods, and it seems we have some design decisions to make about this topic.

We have been choosing whether to expose these arguments on a case by case basis and by applying our own judgement.

As I see it, we have a few choices:

  • Continue applying our own judgement on a case by case basis
  • Expose every argument
  • Expose only the required arguments
  • Add multiple bindings to the same function, some with optional arguments, some without. Do we do every combination of optional arguments? Always require first N arguments are supplied (so N+1 versions of the function for N optional arguments)?
@alexcrichton

This comment has been minimized.

Copy link
Collaborator

alexcrichton commented Jul 24, 2018

I'm less certain about using &JsString all over the place, it's actually a lot faster to use &str if you don't reuse the JsString elsewhere because that way you only cross the wasm boundary once instead of N + 1 (where N is the number of strings). I'm not sure how often one is used over the other, but I'm personally partial to a strategy where we support both (through methods like fn parse(&str) and fn parse_js(&JsString)).

I do agree though we should probably take by-ref everywhere!

For optional arguments I think it's fine to basically cut what we have. It's subpar in some places but it can always be worked around locally if necessary or we can have more releases.

One last thing I've realized is we probably want to #[derive(Clone, Debug)] all the types here as well, but that won't be too hard to do!

@toVersus

This comment has been minimized.

Copy link
Contributor

toVersus commented Jul 25, 2018

This is quite trivial thing, but can we resolve the inconsistency whether semblance of "C" should be declared or not, which @alexcrichton pointed out before in #422? I am using rustfmt and it frustrates me to complement semblance (and put some indentations) whenever applying changes to the code.

@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Jul 25, 2018

@toVersus I personally agree we should just switch to using rustfmt for styling, but it definitely isn't a blocker for publishing the js-sys crate.

This was referenced Jul 25, 2018

@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Jul 26, 2018

Will publish tomorrow, if anyone has any last minute concerns, now is your time to voice them!

@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Jul 26, 2018

@fitzgen fitzgen closed this Jul 26, 2018

@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Jul 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.