Skip to content

Fix js_sys::Intl methods returning Functions instead of calling them directly #2506

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

glennsl
Copy link
Contributor

@glennsl glennsl commented Mar 24, 2021

According to MDN 'Intl.DateTimeFormat.format' takes a Date and returns a String.

I'm not sure why the current binding was made as it is, but the fact that there's a test for it that checks that it does the wrong thing makes it look deliberate. This was originally added in #691, but I see no rationale for it there either. Perhaps there's some obscure historical reason for it. I do see several similar mistakes in the Intl namespace however, but figured I'd start with something small and simple to make sure I'm not misunderstanding something.

Also, the value returned apparently isn't an instance of JsString, but is_string() still returns true so I'm hoping that's fine.

@glennsl
Copy link
Contributor Author

glennsl commented Mar 24, 2021

According to the the specification it actually is a getter that returns a DateTime format function. That happens to work out well in JavaScript where it's indistinguishable from a method. It's not so useful as a rust binding though.

Perhaps an argument can be made for a format_function binding in addition to this, but I don't really see it. I reckon you could just as well make a closure of your own to call this.

@alexcrichton
Copy link
Contributor

Thanks for the PR! Unfortunately this is a breaking change so it can't land just yet, but I'll tag it as such for now.

@alexcrichton alexcrichton added breaking-change Tracking breaking changes for the next major version bump (if ever) js-sys Issues related to the `js-sys` crate labels Mar 29, 2021
@glennsl
Copy link
Contributor Author

glennsl commented Mar 29, 2021

No problem. Would you like me to do the same with the other similar APIs in the Intl namespace? As they'd also be breaking changes it would be good to get them sooner rather than later.

Also, is there any kind of ETA on a breaking release? Judging by the history it seems it might take a while. And I do need these APIs, so perhaps it would be better to make a local copy.

@alexcrichton
Copy link
Contributor

If you'd like feel free!

Unfortunately there is no eta on a new breaking release

@glennsl glennsl changed the title Fix js_sys::Intl::DateTimeFormat::format Fix js_sys::Intl methods returning Functions instead of calling them directly Apr 1, 2021
@glennsl
Copy link
Contributor Author

glennsl commented Apr 1, 2021

Done.

@glennsl
Copy link
Contributor Author

glennsl commented Apr 1, 2021

Would it also be good to make the options objects strongly typed? I've tried to mimic the way it's done in web-sys, but I'm getting failed to resolve: could not find `js_sys` in `{{root}}` on wasm_bindgen attributes attached to enums. Perhaps it wasn't designed to be used from within js-sys itself. And I'd rather not waste a lot of time looking into this if it wouldn't be all that beneficial anyway.

@alexcrichton
Copy link
Contributor

I think it's probably fine to leave as-is for now, but thanks!

@MendyBerger
Copy link

MendyBerger commented Jan 24, 2024

Since it doesn't look like this will be fixed any time soon.
You can just call .call1 on the format function to get the current version to work.

let locales = js_sys::Array::of1(&JsValue::from("en-US"));
let options = js_sys::Object::new();
let intl = js_sys::Intl::NumberFormat::new(&locales, &options);
let num = JsValue::from_f64(140.90);
intl.format().call1(&intl, &num).unwrap().as_string().unwrap()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Tracking breaking changes for the next major version bump (if ever) js-sys Issues related to the `js-sys` crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants