Skip to content

Conversation

@Uzlopak
Copy link
Contributor

@UzlopakUzlopak commented Oct 20, 2023

This adds navigator.language and navigator.languages based on the capabilities of ICU.

@anonrig

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. needs-ci PRs that need a full CI run. labels Oct 20, 2023
@GeoffreyBoothGeoffreyBooth added the web-standards Issues and PRs related to Web APIs label Oct 26, 2023
@GeoffreyBooth
Copy link
Member

Let’s please ping @nodejs/web-standards for all issues and PRs related to navigator.

panva
panva previously requested changes Oct 26, 2023
Copy link
Member

@panvapanva left a comment

Choose a reason for hiding this comment

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

This doesn't seem entirely useful given it's based on the capabilities of ICU which for most is bundled.

Consider this a non-blocking -1 that you may dismiss given there's demand for this behaviour.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Oct 29, 2023

This doesn't seem entirely useful given it's based on the capabilities of ICU which for most is bundled.

Consider this a non-blocking -1 that you may dismiss given there’s demand for this behaviour.

@panva can you explain? What do you mean by “given it’s based on the capabilities of ICU which for most is bundled”?

The reason to add these properties, like most of the navigator properties, is to enable code that was written for browsers to run without erroring in Node. All of the navigator properties are duplicative of existing Node APIs that provide the same information; that’s irrelevant, because the point is to support code that isn’t written for Node.

@UzlopakUzlopakforce-pushed the navigatorLanguageS branch 2 times, most recently from 332aa0a to 74ac025CompareOctober 29, 2023 02:35
@Uzlopak
Copy link
ContributorAuthor

I thought about this PR and simplified it.

Can you set the default locale of icu in node? If yes, than this PR is still wrong, as it should use that value instead.
But if you cant set the default locale of icu, than this PR should be correct.

@GeoffreyBooth
Copy link
Member

Can you set the default locale of icu in node? If yes, than this PR is still wrong, as it should use that value instead.

Yes, you can:

LC_ALL=en_US node --print 'new Date().toLocaleString()' 10/28/2023, 8:13:49 PM LC_ALL=en_UK node --print 'new Date().toLocaleString()' 10/28/2023, 20:13:55

I don’t know if LC_ALL is the only way to set it, but it’s at least one way. You can use Intl.DateTimeFormat().resolvedOptions().locale to see what Node is using:

LC_ALL=en_US node --print 'Intl.DateTimeFormat().resolvedOptions().locale' en-US LC_ALL=en_UK node --print 'Intl.DateTimeFormat().resolvedOptions().locale' en-UK 

@panvapanva dismissed their stale reviewOctober 29, 2023 08:08

It was non-blocking in the first place. If the new approach just returns Intl.DateTimeFormat().resolvedOptions().locale it's fine.

@UzlopakUzlopakforce-pushed the navigatorLanguageS branch 3 times, most recently from ec11b6d to f0ab0c0CompareOctober 29, 2023 13:01
@Uzlopak
Copy link
ContributorAuthor

@GeoffreyBooth

Nice. LOL, you dont know how much i searched around to find this retrieval opportunity.

Falling back to 'en-US' if Intl does not exist. I guess. how can I configure make to build with full intl?

I oriented the functionality based on the behaviour of Chrome and Firefox. So if you use defineProperty to overwrite navigator.language, then languages wont be changed, like in the browser. Also the Array of navigator.languages is frozen, like it is in chrome and firefox.

Can someone please add the 'author-ready' label and remove the 'c++' and 'i18n-api' labels

Copy link
Member

@GeoffreyBoothGeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Let's add a test that uses spawnPromisified to start a Node process with an LC_ALL of something non-default, and see that it carries through to this new property.

We should also mention in the docs that this environment variable is one way to set Node's default language.

@GeoffreyBooth
Copy link
Member

how can I configure make to build with full intl?

See https://github.com/nodejs/node/blob/65087c04866dad805554102592754f1262bfb3b2/doc/api/intl.md

@UzlopakUzlopakforce-pushed the navigatorLanguageS branch 2 times, most recently from fed4162 to 701237aCompareOctober 30, 2023 00:35
@Uzlopak
Copy link
ContributorAuthor

@panva
@jasnell
@GeoffreyBooth
@anonrig

PTAL

And if somebody approves, please add then the author-ready label :)

@GeoffreyBoothGeoffreyBooth added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBoothGeoffreyBooth added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Nov 4, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 4, 2023
@nodejs-github-botnodejs-github-bot merged commit 77b0595 into nodejs:mainNov 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 77b0595

@UzlopakUzlopak deleted the navigatorLanguageS branch November 5, 2023 12:02
anonrig pushed a commit to anonrig/node that referenced this pull request Nov 9, 2023
PR-URL: nodejs#50303 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #50303 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
@targostargos mentioned this pull request Nov 12, 2023
@targostargos added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 13, 2023
@targos
Copy link
Member

This is semver-minor. Collaborators, please think about adding the label in future pull requests. It will make the work of releasers easier.

targos added a commit that referenced this pull request Nov 13, 2023
Notable changes: doc: * add MrJithil to collaborators (Jithil P Ponnan) #50666 * add Ethan-Arrowood as a collaborator (Ethan Arrowood) #50393 esm: * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) #48740 fs: * add stacktrace to fs/promises (翠 / green) #49849 lib: * (SEMVER-MINOR) add `--no-experimental-global-navigator` CLI flag (Antoine du Hamel) #50562 * (SEMVER-MINOR) add navigator.language & navigator.languages (Aras Abbasi) #50303 * (SEMVER-MINOR) add navigator.platform (Aras Abbasi) #50385 stream: * (SEMVER-MINOR) add support for `deflate-raw` format to webstreams compression (Damian Krzeminski) #50097 * use Array for Readable buffer (Robert Nagy) #50341 * optimize creation (Robert Nagy) #50337 test_runner: * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) #50018 * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) #48638 test_runner, cli: * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) #50443 PR-URL: #50681
targos pushed a commit that referenced this pull request Nov 14, 2023
PR-URL: #50303 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
targos added a commit that referenced this pull request Nov 14, 2023
Notable changes: doc: * add MrJithil to collaborators (Jithil P Ponnan) #50666 * add Ethan-Arrowood as a collaborator (Ethan Arrowood) #50393 esm: * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) #48740 fs: * add stacktrace to fs/promises (翠 / green) #49849 lib: * (SEMVER-MINOR) add `--no-experimental-global-navigator` CLI flag (Antoine du Hamel) #50562 * (SEMVER-MINOR) add navigator.language & navigator.languages (Aras Abbasi) #50303 * (SEMVER-MINOR) add navigator.platform (Aras Abbasi) #50385 stream: * (SEMVER-MINOR) add support for `deflate-raw` format to webstreams compression (Damian Krzeminski) #50097 * use Array for Readable buffer (Robert Nagy) #50341 * optimize creation (Robert Nagy) #50337 test_runner: * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) #50018 * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) #48638 test_runner, cli: * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) #50443 PR-URL: #50681
targos added a commit that referenced this pull request Nov 14, 2023
Notable changes: doc: * add MrJithil to collaborators (Jithil P Ponnan) #50666 * add Ethan-Arrowood as a collaborator (Ethan Arrowood) #50393 esm: * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) #48740 fs: * add stacktrace to fs/promises (翠 / green) #49849 lib: * (SEMVER-MINOR) add `--no-experimental-global-navigator` CLI flag (Antoine du Hamel) #50562 * (SEMVER-MINOR) add navigator.language & navigator.languages (Aras Abbasi) #50303 * (SEMVER-MINOR) add navigator.platform (Aras Abbasi) #50385 stream: * (SEMVER-MINOR) add support for `deflate-raw` format to webstreams compression (Damian Krzeminski) #50097 * use Array for Readable buffer (Robert Nagy) #50341 * optimize creation (Robert Nagy) #50337 test_runner: * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) #50018 * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) #48638 test_runner, cli: * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) #50443 PR-URL: #50681
martenrichter pushed a commit to martenrichter/node that referenced this pull request Nov 26, 2023
Notable changes: doc: * add MrJithil to collaborators (Jithil P Ponnan) nodejs#50666 * add Ethan-Arrowood as a collaborator (Ethan Arrowood) nodejs#50393 esm: * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) nodejs#48740 fs: * add stacktrace to fs/promises (翠 / green) nodejs#49849 lib: * (SEMVER-MINOR) add `--no-experimental-global-navigator` CLI flag (Antoine du Hamel) nodejs#50562 * (SEMVER-MINOR) add navigator.language & navigator.languages (Aras Abbasi) nodejs#50303 * (SEMVER-MINOR) add navigator.platform (Aras Abbasi) nodejs#50385 stream: * (SEMVER-MINOR) add support for `deflate-raw` format to webstreams compression (Damian Krzeminski) nodejs#50097 * use Array for Readable buffer (Robert Nagy) nodejs#50341 * optimize creation (Robert Nagy) nodejs#50337 test_runner: * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) nodejs#50018 * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) nodejs#48638 test_runner, cli: * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) nodejs#50443 PR-URL: nodejs#50681
@richardlaurichardlau added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Mar 25, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.dont-land-on-v20.xPRs that should not land on the v20.x-staging branch and should not be released in v20.x.i18n-apiIssues and PRs related to the i18n implementation.needs-ciPRs that need a full CI run.semver-minorPRs that contain new features and should be released in the next minor version.web-standardsIssues and PRs related to Web APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@Uzlopak@GeoffreyBooth@nodejs-github-bot@targos@ljharb@panva@jasnell@anonrig@tniessen@richardlau