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

Move Intl.Collator caseFirst option under the constructor #6253

Merged
merged 1 commit into from Jun 11, 2020
Merged

Move Intl.Collator caseFirst option under the constructor #6253

merged 1 commit into from Jun 11, 2020

Conversation

gilmoreorless
Copy link
Contributor

This is a follow-up to the discussion in #6202. There was an inconsistency in the data structure regarding options to Intl object constructors. The options for Intl.DateTimeFormat() are nested under the constructor, but the option for Intl.Collator() was at the same level as the constructor.

As discussed with @ddbeck, this PR moves the caseFirst option so that it's nested underneath the Intl.Collator() constructor. This might be considered a breaking change, though.

@ghost ghost added the data:js 📟 Compat data for JS/ECMAScript features. https://developer.mozilla.org/docs/Web/JavaScript label Jun 6, 2020
@gilmoreorless
Copy link
Contributor Author

Ooh, looks like the tests have picked up other inconsistencies with the pre-existing data for Android, now that it's nested. The current data says that webview_android supports an option for a constructor that it doesn't have, and there's a version mismatch for chrome_android too.

Now that I look closely, I see the data for the caseFirst option is also incorrect for Safari. The data file says it's not supported, but it works when I try it locally. Likewise for Edge.

I think I'll need to do some digging to find out the correct versions before I can fix up this PR.

@gilmoreorless
Copy link
Contributor Author

I've raised #6260 to correct the data. Once (if) that's merged, I'll rebase this PR to get it passing the tests. (Sorry, I thought I'd run the tests locally first, but clearly I didn't.)

@gilmoreorless
Copy link
Contributor Author

I've rebased this on top of the fixed data from #6260 so hopefully it's good to go now. I also found some prior work in moving constructor options to be nested underneath the constructor in #5750, so I'm slightly more confident in the change. 😉

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Looks like I forgot this back then. Thanks for your PR! 👍

@Elchi3 Elchi3 merged commit 8e4a282 into mdn:master Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:js 📟 Compat data for JS/ECMAScript features. https://developer.mozilla.org/docs/Web/JavaScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants