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

Copy [[PluralCategories]] in Intl.PluralRules.prototype.resolvedOptions? #224

Closed
anba opened this issue Mar 16, 2018 · 6 comments
Closed

Comments

@anba
Copy link
Contributor

anba commented Mar 16, 2018

Follow-up from tc39/proposal-intl-plural-rules#28 (comment)

Seems like I've misinterpreted the SpiderMonkey implementation, because SpiderMonkey definitely returns a copy of the [[PluralCategories]] array in Intl.PluralRules.prototype.resolvedOptions(): https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/js/src/builtin/intl/PluralRules.js#235-240

@littledan
Copy link
Member

I thought we settled on not copying the array. Sorry if I didn't document this clearly in bug threads, but reviewing that thread, it looks like we settled on not copying there. Do we want to reconsider this decision? @caridy @zbraniecki @thejoshwolfe

@zbraniecki
Copy link
Member

I don't have a strong opinion here.

@anba
Copy link
Contributor Author

anba commented Apr 12, 2018

I thought we settled on not copying the array.

Well, I thought SM didn't copy the array (tc39/proposal-intl-plural-rules#28 (comment)), but actually SM does the copy the array and it did so since day one. That's why I wanted to discuss this issue again. And always returning a new array is consistent with how resolvedOptions() works in general, that is it always returns a new object.

@littledan
Copy link
Member

I'd recommend not copying, per the resolution in tc39/proposal-intl-plural-rules#28 , and we can pursue a specification change separately if you want to make the case for it. If there are no test262 tests that verify not copying, then it'd be great if someone added one.

@littledan
Copy link
Member

littledan commented Apr 25, 2018

We discussed this in the April 2018 Intl call and the conclusion was to return a new array each time.

@littledan
Copy link
Member

Drafted spec text for this change at #233 . Does anyone want to create test262 tests to verify?

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

No branches or pull requests

3 participants