intl: Add more versions from ICU by srl295 · Pull Request #9266 · nodejs/node · GitHub

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

intl, bootstrap_node.js

Description of change

Adds process.versions.cldr, .tz, and .unicode
Changes how process.versions.icu is loaded
Lazy loads the process.versions.* values for these
Note: "node -p process.versions" shows "[Getter]" for these values

I usually find if (!icu) return; a bit easier to read, but maybe that’s only because it avoids the extra indentation level, so feel free to keep this as it is.

I think a comma-separated list is fine for this, but maybe the preceding comment should contain a quick reminder that icu.getVersion() without arguments returns the list of available different versions.

I think you can just drop this line

Hm. I'd think the original caller wouldn't get a value, but I'll try it.

@srl295 I’m referring to the comment, in case that’s not clear :)

> process.versions.icu
undefined
> process.versions.icu
'57.1'

so, I think I need this…

@srl295 what part did you remove? What I meant was the // set: setReal, comment, because that’s a leftover from copying, and it seems a bit confusing given that there is no setReal defined here

I think the prevalent style in this codebase is having the * on the left, i.e. const char* everywhere

Could you indent this so that the parameters align vertically?

giving up on trying to use the ICU convention of "UErrorCode& status" here… I can't keep the NOLINT, arg indent, AND stay under the line width. What does cpplint have against non-const references? 👎

I would probably prefer an explicit return NULL; in the case of failure here.

(whispers) nullptr

well, this isn't good:

$ configure --without-intl
$ ./node 
bootstrap_node.js:310
    const icu = process.binding('icu');
                        ^

Error: No such module: icu
    at setupProcessICUVersions (bootstrap_node.js:310:25)
    at startup (bootstrap_node.js:29:5)
    at bootstrap_node.js:533:3

What's that about breaking non-intl builds? Looks like from url.js it seems I need a try/catch here… or is it better to check v8_enable_i18n_support or something?

Looks like from url.js it seems I need a try/catch here

Sounds reasonable to me

Note: "node -p process.versions" shows "[Getter]" for these values

Yeah, we probably should override util.inspect for process.versions. There’s some info about how to do that in the docs and a WIP PR with a more sophisticated example, if it helps.

Or, if somebody else thinks that makes sense, we could add another Symbol that could be used for tagging objects so that util.inspect knows that it should resolve getters on that object, e.g.:

const foobar = Object.create({}, {
  a: {
    get() { return 42; },
    enumerable: true
  }
});

console.log(foobar); // prints `{ a: [Getter] }`

foobar[util.inspect.resolveGetters] = true;

console.log(foobar); // prints `{ a: 42 }`

@addaleax I'll look at those examples, and push what I have. I'd probably go for overriding in this PR and it can be made a tag later? Seems like that would want some more design than the low-level stuff I'm doing now.

Update: It doesn't seem to work to require('util') from within bootstrap_node.js …

This sort of works, but I don't know where it can be executed:

process.versions[util.inspect.custom] = (depth) => JSON.stringify(process.versions);

update update i found a place to run it…

Also , rebased because of #9038

You can also check process.binding('config').hasIntl here... e.g.

const icu = process.binding('config').hasIntl ?
  process.binding('icu') : undefined;

yes… that's what I wanted.

This shouldn't be necessary to set explicitly. The function will return undefined automatically if no return value is set.

by the way - I verified with lsof that ICU data is not loaded until after process.versions.cldr, etc, is called. So, the laziness is working.

Why not have icu.getVersion() simply return an Array? Or perhaps (even better) an object with the keys and values already set so that there is only one call to the binding layer?

Oh... I see... nevermind, lazy loading and all that...

Yeah, it could return an Array from C++ instead.

Just making sure - should this be "if intlEnabled"? It doesn't need to be, but just a question.

* Adds process.versions.cldr, .tz, and .unicode * Changes how process.versions.icu is loaded * Lazy loads the process.versions.* values for these * add an exception to util.js to cause 'node -p process.versions' to still work * update process.version docs Fixes: nodejs#9237

Landed in 4fb27d4

@sam-github pointed out that 4fb27d4 was landed with bad formatting.

The PR and Reviewed-By metadata didn't get added during merge.

@srl295 did you get the collaborator onboarding? Or was it long ago, and you forgot? :-)

* Adds process.versions.cldr, .tz, and .unicode * Changes how process.versions.icu is loaded * Lazy loads the process.versions.* values for these * add an exception to util.js to cause 'node -p process.versions' to still work * update process.version docs Fixes: #9237

@srl295 Since I’ve found it helpful that others added the metadata as a github comment on commits where I had forgotten to add it, I’ve gone ahead and did the same + took the liberty to add me as a reviewer. :)

(Also: Is this semver-minor?)

Yes, this would be semver-minor, good catch. I added the label.

Thanks and yes minor

On Friday, October 28, 2016, James M Snell notifications@github.com wrote:

Yes, this would be semver-minor, good catch. I added the label.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9266 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA0Ms5-Lz-YtCXwpHMtT-2fImF87w6ISks5q4pjigaJpZM4Kfeak
.

* Adds process.versions.cldr, .tz, and .unicode * Changes how process.versions.icu is loaded * Lazy loads the process.versions.* values for these * add an exception to util.js to cause 'node -p process.versions' to still work * update process.version docs PR-URL: #9266 Fixes: #9237 Reviewed-By: James M Snell <jasnell@gmail.com>

adding don't land for v4.x

is this something we would want to consider in a future minor to v6?

is this something we would want to consider in a future minor to v6?

ping @srl295

Should land with #13221 if it lands on v6.x.

srl295 self-assigned this Oct 25, 2016

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 25, 2016

srl295 force-pushed the icu-more-versions branch 3 times, most recently from be6c2d7 to 1ee56e5 Compare October 25, 2016 02:27

srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Oct 25, 2016

addaleax reviewed Oct 25, 2016

View reviewed changes

srl295 force-pushed the icu-more-versions branch 2 times, most recently from ad20744 to 9b5a808 Compare October 25, 2016 18:10

jasnell reviewed Oct 25, 2016

View reviewed changes

jasnell approved these changes Oct 25, 2016

View reviewed changes

srl295 commented Oct 25, 2016

View reviewed changes

srl295 force-pushed the icu-more-versions branch from ed54858 to e2a8cce Compare October 27, 2016 02:23

srl295 closed this Oct 27, 2016

srl295 deleted the icu-more-versions branch October 27, 2016 02:28

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 29, 2016

evanlucas mentioned this pull request Nov 3, 2016

v7.1.0 proposal - 2016-11-08 #9438

Merged

MylesBorins added dont-land-on-v4.x labels Nov 22, 2016

MylesBorins added the lts-agenda label Dec 2, 2016

MylesBorins mentioned this pull request Jan 6, 2017

Potential Semver Minor Backports nodejs/Release#177

Closed

9 tasks

MylesBorins added dont-land-on-v6.x and removed lts-watch-v6.x labels Jan 23, 2017

gibfahn mentioned this pull request Jun 17, 2017

Review semver-minor backports nodejs/Release#228

Closed

8 tasks

JacksonTian mentioned this pull request Sep 22, 2017

lib: simplify the readonly properties of icu #13221

Closed

3 tasks

seishun mentioned this pull request Nov 17, 2017

repl: strange regression in output colors #17086

Closed

intl: Add more versions from ICU by srl295 · Pull Request #9266 · nodejs/node · GitHub

Uh oh!

Uh oh!

intl: Add more versions from ICU - #9266

intl: Add more versions from ICU#9266srl295 wants to merge 1 commit intonodejs:masternodejs/node:masterfrom srl295:icu-more-versionssrl295/node:icu-more-versionsCopy head branch name to clipboard

Conversation

srl295 commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srl295 commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Oct 25, 2016

Uh oh!

srl295 commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srl295 commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srl295 commented Oct 27, 2016

Uh oh!

srl295 commented Oct 27, 2016

Uh oh!

sam-github commented Oct 27, 2016

Uh oh!

addaleax commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Oct 29, 2016

Uh oh!

srl295 commented Oct 29, 2016

Uh oh!

MylesBorins commented Nov 22, 2016

Uh oh!

gibfahn commented Jun 17, 2017

Uh oh!

gibfahn commented Sep 24, 2017

Uh oh!

Reviewers

intl: Add more versions from ICU#9266
srl295 wants to merge 1 commit into
nodejs:masterfrom
srl295:icu-more-versions

srl295 commented Oct 25, 2016 •
edited
Loading

srl295 commented Oct 25, 2016 •
edited
Loading

srl295 commented Oct 25, 2016 •
edited
Loading

addaleax commented Oct 28, 2016 •
edited
Loading