intl: Add more versions from ICU

[srl295]

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

Fixes: #9237

[addaleax]

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.

[addaleax]

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.

[addaleax]

I think you can just drop this line

[srl295]

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

[addaleax]

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

[srl295]

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

so, I think I need this…

[addaleax]

@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

[addaleax]

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

[addaleax]

Could you indent this so that the parameters align vertically?

[srl295]

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? 👎

[addaleax]

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

[bnoordhuis]

(whispers) nullptr

[srl295]

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?

[addaleax]

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.:

constfoobar=Object.create({},{a: {get(){return42;},enumerable: true}});console.log(foobar);// prints `{ a: [Getter] }`foobar[util.inspect.resolveGetters]=true;console.log(foobar);// prints `{ a: 42 }`

[srl295]

@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

[jasnell]

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

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

[srl295]

yes… that's what I wanted.

[jasnell]

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

[srl295]

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

[jasnell]

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?

[jasnell]

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

[srl295]

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

[srl295]

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

[srl295]

Landed in 4fb27d4

[srl295]

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

[sam-github]

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? :-)

[addaleax]

@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?)

[jasnell]

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

[srl295]

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
.

[MylesBorins]

adding don't land for v4.x

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

[gibfahn]

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

ping @srl295

[gibfahn]

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