Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

fix: Intl.getCanonicalLocales error message #2741

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

Merged
merged 4 commits into from
Feb 29, 2024

Conversation

btea
Copy link
Contributor

@btea btea commented Feb 21, 2024

Description

Motivation

Additional details

Related issues and pull requests

@btea btea requested a review from a team as a code owner February 21, 2024 14:28
@btea btea requested review from Josh-Cena and removed request for a team February 21, 2024 14:28
Copy link

It looks like this is your first pull request. 🎉 Thank you for your contribution! One of the project maintainers will triage and assign the pull request for review. We appreciate your patience. To safeguard the health of the project, please take a moment to read our code of conduct.

@bsmth
Copy link
Member

bsmth commented Feb 22, 2024

Thanks a lot for raising this one. I'm going to close, however, because I get the following:

image

The error message here is browser-specific so it's safe enough to leave as-is.

Thanks!

@bsmth bsmth closed this Feb 22, 2024
@btea
Copy link
Contributor Author

btea commented Feb 23, 2024

@bsmth Is it because of the browser difference? The error message I get is as follows:

image

The browser version I am using is Edge 121.0.2277.128.

@bsmth
Copy link
Member

bsmth commented Feb 23, 2024

Yes, I can confirm if you try it with Chrome, you get the following:

> Array ["en-US"]
> Array ["en-US", "fr"]
> "RangeError: Incorrect locale information provided"

And Safari:

> Array ["en-US"]
> Array ["en-US", "fr"]
> "RangeError: invalid language tag: EN_US"

@btea
Copy link
Contributor Author

btea commented Feb 23, 2024

The error messages of Firefox and Safari are consistent, and the output messages of Chrome and node are consistent. Maybe add both error messages and then mark the running environment?

@bsmth
Copy link
Member

bsmth commented Feb 23, 2024

We can add both to cover Chrome, Fx and Safari, why not. This can also be verified using eshost

'Intl.getCanonicalLocales("EN_US")'
┌──────────────┬───────────────────────────────────────────────────┐
│ SpiderMonkey │                                                   │
│              │ RangeError: invalid language tag: "EN_US"         │
├──────────────┼───────────────────────────────────────────────────┤
│ V8           │                                                   │
│              │ RangeError: Incorrect locale information provided │
└──────────────┴───────────────────────────────────────────────────┘

mark the running environment

The running environment is the reader's browser in this case, so I don't think we need to do anything else there.

@bsmth bsmth reopened this Feb 23, 2024
live-examples/js-examples/intl/intl-getcanonicallocales.js Outdated Show resolved Hide resolved
Co-authored-by: Brian Thomas Smith <brian@smith.berlin>
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Thank you!

@Josh-Cena
Copy link
Member

Do we do this elsewhere?

I don't care much for these examples but I also don't think it's helpful when the message has the same meaning anyway.

@bsmth
Copy link
Member

bsmth commented Feb 26, 2024

Do we do this elsewhere?

We do have a few:

live-examples/js-examples/intl/intl-listformat-prototype-resolvedoptions.js:// Expected output (Chrome): "de"
live-examples/js-examples/intl/intl-numberformat-prototype-resolvedoptions.js:// Expected output (Chrome): "de"
live-examples/js-examples/weakset/weakset-prototype-add.js:  // Expected output (Chrome): TypeError: Invalid value used in weak set
live-examples/js-examples/symbol/symbol-match.js:// Expected output (Chrome): Error: First argument to String.prototype.startsWith must not be a regular expression

And actually I just spotted we have this in the style guide: https://github.com/mdn/interactive-examples/blob/main/contributing/javascript-style-guide.md#representing-browser-differences

I also don't think it's helpful when the message has the same meaning anyway

I tend to agree somewhat, my initial decision was to close for this reason, but I can see why some readers might be confused. I would format it like the suggestion in the style guide and merge, unless you have objections, @Josh-Cena.

@Josh-Cena
Copy link
Member

That "browser difference" is actual runtime difference, but this one is just a representational difference. I don't have strong objections, though.

live-examples/js-examples/intl/intl-getcanonicallocales.js Outdated Show resolved Hide resolved
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Thank you!

@bsmth
Copy link
Member

bsmth commented Feb 29, 2024

That "browser difference" is actual runtime difference, but this one is just a representational difference. I don't have strong objections, though.

You're right. Let's go with this for now 👍🏻

@bsmth bsmth merged commit f5a8492 into mdn:main Feb 29, 2024
Copy link

Congratulations on your first merged pull request. 🎉 Thank you for your contribution! Did you know we have a project board with high-impact contribution opportunities? We look forward to your next contribution.

@btea btea deleted the fix/intl-getcanonicallocales-error-message branch February 29, 2024 12:11
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.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.