intl: Add more versions from ICU #9266

srl295 · 2016-10-25T01:50:17Z

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 · 2016-10-25T13:20:43Z

lib/internal/bootstrap_node.js

@@ -304,6 +306,40 @@
    };
  }

+  function setupProcessICUVersions() {
+    const icu = process.binding('icu');
+    if (icu) {


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 · 2016-10-25T13:23:54Z

lib/internal/bootstrap_node.js

+      const object = process.versions;
+      // if ICU is present, fetch additional versions
+      // TODO(srl295): should this be a comma separated list or something else?
+      const versionTypes = icu.getVersion().split(',');


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 · 2016-10-25T13:27:13Z

lib/internal/bootstrap_node.js

+
+            return realValue;
+          },
+          // set: setReal,


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

addaleax · 2016-10-25T13:28:04Z

src/node_i18n.cc

+ * @param status ICU error status. If failure, assume result is undefined.
+ * @return version number, or NULL. May or may not be buf.
+ */
+static const char *GetVersion(const char *type,


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

addaleax · 2016-10-25T13:28:46Z

src/node_i18n.cc

+ * @return version number, or NULL. May or may not be buf.
+ */
+static const char *GetVersion(const char *type,
+char buf[U_MAX_VERSION_STRING_LENGTH],


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

addaleax · 2016-10-25T13:35:25Z

src/node_i18n.cc

+    if (U_SUCCESS(status)) {
+      u_versionToString(versionArray, buf);
+    }
+    return buf;


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

(whispers) nullptr

srl295 · 2016-10-25T15:19:15Z

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 · 2016-10-25T16:42:44Z

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 }`

srl295 · 2016-10-25T17:38:24Z

@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 · 2016-10-25T18:19:38Z

lib/internal/bootstrap_node.js

@@ -304,6 +306,40 @@
    };
  }

+  function setupProcessICUVersions() {
+    var icu;


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.

jasnell · 2016-10-25T18:21:34Z

src/node_i18n.cc

+          String::NewFromUtf8(env->isolate(),
+          versionString));
+    } else {
+      // Fail => undefined


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

srl295 · 2016-10-25T18:39:08Z

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.

jasnell · 2016-10-25T21:02:36Z

lib/internal/bootstrap_node.js

+    if (!icu) return;  // no Intl/ICU: nothing to add here.
+    // With no argument, getVersion() returns a comma separated list
+    // of possible types.
+    const versionTypes = icu.getVersion().split(',');


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.

srl295 · 2016-10-25T22:32:55Z

lib/util.js

+
+// process.versions needs a custom function as some values are lazy-evaluated.
+process.versions[exports.inspect.custom] =
+  (depth) => exports.format(JSON.parse(JSON.stringify(process.versions)));


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

srl295 · 2016-10-27T02:27:59Z

Landed in 4fb27d4

srl295 · 2016-10-27T18:41:36Z

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

sam-github · 2016-10-27T18:56:15Z

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

addaleax · 2016-10-28T21:18:51Z

@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 · 2016-10-29T00:52:38Z

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

srl295 · 2016-10-29T22:55:13Z

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>

MylesBorins · 2016-11-22T07:10:14Z

adding don't land for v4.x

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

gibfahn · 2017-06-17T19:52:39Z

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

ping @srl295

gibfahn · 2017-09-24T15:20:30Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

intl: Add more versions from ICU #9266

intl: Add more versions from ICU #9266

srl295 commented Oct 25, 2016 •

edited

Loading

addaleax Oct 25, 2016

addaleax Oct 25, 2016

addaleax Oct 25, 2016

srl295 Oct 25, 2016

addaleax Oct 25, 2016

srl295 Oct 25, 2016

addaleax Oct 25, 2016

addaleax Oct 25, 2016

addaleax Oct 25, 2016

srl295 Oct 25, 2016

addaleax Oct 25, 2016

bnoordhuis Oct 25, 2016

srl295 commented Oct 25, 2016 •

edited

Loading

addaleax commented Oct 25, 2016

srl295 commented Oct 25, 2016 •

edited

Loading

jasnell Oct 25, 2016

srl295 Oct 25, 2016

jasnell Oct 25, 2016

srl295 commented Oct 25, 2016

jasnell Oct 25, 2016

jasnell Oct 25, 2016

srl295 Oct 25, 2016

srl295 Oct 25, 2016

srl295 commented Oct 27, 2016

srl295 commented Oct 27, 2016

sam-github commented Oct 27, 2016

addaleax commented Oct 28, 2016 •

edited

Loading

jasnell commented Oct 29, 2016

srl295 commented Oct 29, 2016

MylesBorins commented Nov 22, 2016

gibfahn commented Jun 17, 2017

gibfahn commented Sep 24, 2017

intl: Add more versions from ICU #9266

intl: Add more versions from ICU #9266

Conversation

srl295 commented Oct 25, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 commented Oct 25, 2016 • edited Loading

addaleax commented Oct 25, 2016

srl295 commented Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 commented Oct 27, 2016

srl295 commented Oct 27, 2016

sam-github commented Oct 27, 2016

addaleax commented Oct 28, 2016 • edited Loading

jasnell commented Oct 29, 2016

srl295 commented Oct 29, 2016

MylesBorins commented Nov 22, 2016

gibfahn commented Jun 17, 2017

gibfahn commented Sep 24, 2017

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