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

Normative: Use ListFormat for Array.prototype.toLocaleString #615

Closed
wants to merge 1 commit into from

Conversation

FrankYFTang
Copy link
Contributor

This PR address #422

Copy link
Contributor

@anba anba left a comment

Choose a reason for hiding this comment

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

The options parameter conflict described in #422 is still unresolved.

@bakkot
Copy link
Contributor

bakkot commented Oct 25, 2021

Currently, ['x', 'y', 'z'].toLocaleString(['not a valid locale']) succeeds. With this PR, it would start throwing. I'm not confident this would be web-compatible.

@sffc
Copy link
Contributor

sffc commented Oct 25, 2021

Currently, ['x', 'y', 'z'].toLocaleString(['not a valid locale']) succeeds. With this PR, it would start throwing. I'm not confident this would be web-compatible.

Is this so?

Note that we already call toLocaleString on each item in the list, which means we are already passing locales through any locale validation we have in 402.

  1. Let S be ? ToString(? Invoke(nextElement, "toLocaleString", « locales, options »)).

Can you clarify how we would be having a behavior change?

@bakkot
Copy link
Contributor

bakkot commented Oct 26, 2021

Note that we already call toLocaleString on each item in the list, which means we are already passing locales through any locale validation we have in 402.

There is no String.prototype.toLocaleString, so the invocations in question inherit the toLocaleString method from Object.prototype. 402 does not give an alternate definition for that method, so it uses the one in 262, which does not consume the locales parameter at all.

That is: the current behavior, in the example I gave, does not validate or otherwise consume the locales parameter. With this PR, it would be validated.

@sffc
Copy link
Contributor

sffc commented Oct 26, 2021

toLocaleString is invoked on nextElement, which is not necessarily a string. It could be a number or a date, e.g.:

> [6543.21, " ", new Date()].toLocaleString("de")
// '6.543,21, ,25.10.2021, 18:23:51'
> navigator.userAgent
// 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/94.0.4606.81 Safari/537.36'

@bakkot
Copy link
Contributor

bakkot commented Oct 26, 2021

Right, but in the particular example I gave, nextElement is always string. You can run ['x', 'y', 'z'].toLocaleString(['not a valid locale']) today and confirm that it does not throw in any engine. It would throw with this PR.

@ljharb
Copy link
Member

ljharb commented Oct 26, 2021

take the array out of it, 'x'.toLocaleString('not a valid locale') also doesn't throw.

@sffc
Copy link
Contributor

sffc commented Oct 26, 2021

Okay I see now.

I'm not convinced that the new exception, which is an error case anyhow (and is a bug that it doesn't throw already), would be any less web compatible than the fact that we're changing the i18n behavior of the function. Like, I think it's more likely the case that there's software that relies on the output being 1,2,3 that would break when we change it to 1, 2 and 3 than there is that would break with the new exception. Which is a way to say that, all else equal, I'm not personally concerned about the new exception.

@bakkot
Copy link
Contributor

bakkot commented Oct 26, 2021

I think it's much more likely that there is code which relies on that snippet not throwing than that there is code which relies on the particular value it produces, especially given that it has locale in the name. In my experience code which is calling toLocaleString isn't usually parsing the output, but it is very frequently expecting to get output rather than an exception.

@FrankYFTang
Copy link
Contributor Author

[new Date(), new Date()].toLocaleString(['not a valid locale'])
VM142:1 Uncaught RangeError: Incorrect locale information provided
    at Date.toLocaleString (<anonymous>)
    at Array.toLocaleString (<anonymous>)
    at <anonymous>:1:26
(anonymous) @ VM142:1

[new Date(), new Date()].toLocaleString(['en'])
'10/27/2021, 9:03:07 PM,10/27/2021, 9:03:07 PM'

[Number(1), Number(2)].toLocaleString(['not a valid locale'])
VM241:1 Uncaught RangeError: Incorrect locale information provided
    at Number.toLocaleString (<anonymous>)
    at Array.toLocaleString (<anonymous>)
    at <anonymous>:1:24
(anonymous) @ VM241:1
[1,2].toLocaleString(['not a valid locale'])
VM258:1 Uncaught RangeError: Incorrect locale information provided
    at Number.toLocaleString (<anonymous>)
    at Array.toLocaleString (<anonymous>)
    at <anonymous>:1:7
(anonymous) @ VM258:1

[1n, 2n].toLocaleString(['not a valid locale'])
VM320:1 Uncaught RangeError: Incorrect locale information provided
    at BigInt.toLocaleString (<anonymous>)
    at Array.toLocaleString (<anonymous>)
    at <anonymous>:1:10

@bakkot
Copy link
Contributor

bakkot commented Oct 28, 2021

I acknowledge there are different examples than the one I gave which do throw today. However, the one I gave does not.

@FrankYFTang
Copy link
Contributor Author

Everytime we enhance the ECMA402 API it will break thing. For example, as today

(123.45).toLocaleString(["en"], { roundingMode: "invalid roundingMode value"})

will produce
"123.45"
but once the code run on a browser which support Intl.NumberFormat v3, it will throw.

@bakkot
Copy link
Contributor

bakkot commented Oct 28, 2021

I don't expect there is any code like that in the wild today, but I would not be at all surprised to find there is code like the snippet I posted.

@zbraniecki
Copy link
Member

I think there's a delicate balance between using our intuition to foresee webcompat issues and preventing improvements on the ground of our intuitive concerns.

In this case I'm shying in the direction of not blocking this unless we can validate the web compat concern.

Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

+1, I agree with @zbraniecki

@FrankYFTang
Copy link
Contributor Author

We discussed this in the 2021-11-04 TG2 and I feel there are still other concerns which need to be addressed. But I don't think I could address them myself. I would leave this to whoever want to take it from here to work on that.

@sffc
Copy link
Contributor

sffc commented Nov 8, 2021

See the issue #422 for additional discussion

@sffc
Copy link
Contributor

sffc commented May 19, 2022

2022-05-19: This issue is too large for a PR; it should have a proposal. See #422. Volunteers would be appreciated.

@sffc sffc closed this May 19, 2022
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

Successfully merging this pull request may close these issues.

7 participants