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

lib: add navigator.language and navigator.languages #50303

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Oct 20, 2023

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

@anonrig

@nodejs-github-bot nodejs-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
doc/api/globals.md Outdated Show resolved Hide resolved
doc/api/globals.md Outdated Show resolved Hide resolved
lib/internal/navigator.js Outdated Show resolved Hide resolved
lib/internal/navigator.js Show resolved Hide resolved
lib/internal/navigator.js Outdated Show resolved Hide resolved
src/node_i18n.cc Outdated Show resolved Hide resolved
src/node_i18n.cc Outdated Show resolved Hide resolved
test/parallel/test-navigator.js Outdated Show resolved Hide resolved
doc/api/globals.md Outdated Show resolved Hide resolved
@GeoffreyBooth GeoffreyBooth 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

@panva panva 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.

doc/api/globals.md Outdated Show resolved Hide resolved
@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.

@Uzlopak Uzlopak force-pushed the navigatorLanguageS branch 2 times, most recently from 332aa0a to 74ac025 Compare October 29, 2023 02:35
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 29, 2023

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

@panva panva dismissed their stale review October 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.

@Uzlopak Uzlopak force-pushed the navigatorLanguageS branch 3 times, most recently from ec11b6d to f0ab0c0 Compare October 29, 2023 13:01
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 29, 2023

@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

@GeoffreyBooth GeoffreyBooth 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

doc/api/globals.md Outdated Show resolved Hide resolved
@Uzlopak Uzlopak force-pushed the navigatorLanguageS branch 2 times, most recently from fed4162 to 701237a Compare October 30, 2023 00:35
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 30, 2023

@panva
@jasnell
@GeoffreyBooth
@anonrig

PTAL

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

doc/api/globals.md Outdated Show resolved Hide resolved
doc/api/globals.md Show resolved Hide resolved
doc/api/globals.md Outdated Show resolved Hide resolved
doc/api/globals.md Outdated Show resolved Hide resolved
@GeoffreyBooth GeoffreyBooth added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2023
@github-actions github-actions bot 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

@GeoffreyBooth GeoffreyBooth 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-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 4, 2023
@nodejs-github-bot nodejs-github-bot merged commit 77b0595 into nodejs:main Nov 4, 2023
56 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 77b0595

@Uzlopak Uzlopak 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]>
@targos targos 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

targos commented Nov 13, 2023

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
@richardlau richardlau 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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. i18n-api Issues and PRs related to the i18n implementation. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. web-standards Issues and PRs related to Web APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants