Skip to content

lib: simplify the readonly properties of icu - #13221

Closed
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:versions
Closed

lib: simplify the readonly properties of icu#13221
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:versions

Conversation

@JacksonTian

@JacksonTianJacksonTian commented May 25, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

Call Object.defineProperty() twice to set readonly property is
unnecessary.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

lib/internal/bootstrap_node

@nodejs-github-botnodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 25, 2017
@mscdexmscdex added i18n-api Issues and PRs related to the i18n implementation. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels May 25, 2017
@refack

refack commented May 25, 2017

Copy link
Copy Markdown
Contributor

I'd assume this whole mechanism is necessary if the locale is changed in runtime. @nodejs/intl is there a way to do that?

kunalspathak
kunalspathak reviewed May 26, 2017
View reviewed changes
Comment threadtest/parallel/test-process-versions.js Outdated

@kunalspathakkunalspathakMay 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Can you switch to for(var key of expected_keys) { } instead?

@kunalspathak

kunalspathak commented May 26, 2017•
edited
Loading

Copy link
Copy Markdown
Member

I'd assume this whole mechanism is necessary if the locale is changed in runtime.

Good point. But I am wondering if the locale changed at runtime, icu.getVersion(name); would still return the locales present when node instance was bootstrapped.

@JacksonTian

JacksonTian commented May 27, 2017

Copy link
Copy Markdown
ContributorAuthor

Yes, The locale can be changed, but the ICU version not.

@jasnell

jasnell commented Jun 1, 2017

Copy link
Copy Markdown
Member

ping @srl295

@JacksonTian
lib: simplify the readonly properties of icu
5844b92
Call Object.defineProperty() twice to set readonly property is
unnecessary.
@BridgeAR

BridgeAR commented Aug 30, 2017

Copy link
Copy Markdown
Member

I guess this can land as is?

@jasnell

jasnell commented Aug 30, 2017

Copy link
Copy Markdown
Member

With fresh CI, yes I believe it can.

@BridgeAR

BridgeAR commented Aug 30, 2017

Copy link
Copy Markdown
Member

CI https://ci.nodejs.org/job/node-test-pull-request/9892/

@BridgeAR

BridgeAR commented Aug 30, 2017

Copy link
Copy Markdown
Member

Landed in 83a5eef

BridgeAR pushed a commit that referenced this pull request Aug 30, 2017
@JacksonTian@BridgeAR
lib: simplify the readonly properties of icu
83a5eef
Call Object.defineProperty() twice to set readonly property is
unnecessary.
PR-URL: #13221
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
srl295
srl295 reviewed Aug 30, 2017
View reviewed changes

@srl295srl295 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 makes it look too easy

@srl295

srl295 commented Aug 30, 2017

Copy link
Copy Markdown
Member

@refack no, i don't think there's a mechanism to change the default locale at runtime exposed to node.

@JacksonTian
JacksonTian deleted the versions branch September 2, 2017 08:01
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
@JacksonTian@addaleax
lib: simplify the readonly properties of icu
dfb8b9b
Call Object.defineProperty() twice to set readonly property is
unnecessary.
PR-URL: nodejs/node#13221
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@JacksonTian@MylesBorins
lib: simplify the readonly properties of icu
9c11f33
Call Object.defineProperty() twice to set readonly property is
unnecessary.
PR-URL: #13221
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorinsMylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@JacksonTian@MylesBorins
lib: simplify the readonly properties of icu
605d625
Call Object.defineProperty() twice to set readonly property is
unnecessary.
PR-URL: #13221
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins

MylesBorins commented Sep 20, 2017

Copy link
Copy Markdown
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@JacksonTian

JacksonTian commented Sep 22, 2017

Copy link
Copy Markdown
ContributorAuthor

@MylesBorins It seems the pre-condition #9266 hasn't be backported into v6.x-staging.

@gibfahn

gibfahn commented Sep 24, 2017

Copy link
Copy Markdown
Member

Left a note on #9266, marking this as don't land for now so we don't keep triaging it for 6.x.

@gibfahngibfahn mentioned this pull request Sep 24, 2017
Closed
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srl295srl295srl295 left review comments
@jasnelljasnelljasnell approved these changes
@watildewatildewatilde approved these changes
@BridgeARBridgeARBridgeAR approved these changes
+1 more reviewer
@kunalspathakkunalspathakkunalspathak left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

i18n-apiIssues and PRs related to the i18n implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

11 participants

@JacksonTian@refack@kunalspathak@jasnell@BridgeAR@srl295@MylesBorins@gibfahn@watilde@mscdex@nodejs-github-bot