doc,deps: document how to maintain ICU in Node.js by srl295 · Pull Request #30607 · nodejs/node

srl295 · 2019-11-22T23:19:03Z

update v8 guide to mention ICU
move content from the tools/icu/README.md but leave a pointer

Fixes: #26108

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

srl295 · 2019-11-22T23:20:43Z

see also #26090

ryzokuken

Thanks so much!

MylesBorins · 2019-11-22T23:29:21Z

Should we also document how to upload the timezone data? Doesn't seem to be covered in current guide and honestly I had to struggle a bit to get it to work

srl295 · 2019-11-22T23:49:49Z

Should we also document how to upload the timezone data? Doesn't seem to be covered in current guide and honestly I had to struggle a bit to get it to work

Yes.

But do I have a good answer yet? No. I think it should be easier than what you did (though heroic - 👍) , but I need to take some time to figure out the steps.

So I'd like to make that a future PR.

This is just to document the floating patch mechanism itself, plus scaffolding for future docs.

srl295 · 2019-11-23T00:20:37Z

oh whoops. @apaprocki has already created a great tz doc in #30364 , how about I just merge that in ?

vsemozhetbyt

Some nits.

targos · 2019-11-23T13:20:55Z

Is this needed?

srl295 · 2019-11-23T20:12:29Z

Oops no. A thousand times no. Get Outlook for Android<https://aka.ms/ghei36>

…

________________________________ From: Michaël Zasso <notifications@github.com> Sent: Saturday, November 23, 2019 5:20:58 AM To: nodejs/node <node@noreply.github.com> Cc: Steven R. Loomis <srl295@gmail.com>; Assign <assign@noreply.github.com> Subject: Re: [nodejs/node] doc,deps: document how to maintain ICU in Node.js (#30607) Is this needed? [image]<https://user-images.githubusercontent.com/2352663/69479349-755a6700-0dfc-11ea-912d-56d22c17f60d.png> — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub<#30607?email_source=notifications&email_token=AAGQZM53NDH2CZECOPHEUWLQVEU3VA5CNFSM4JQXCY52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEE7VBFI#issuecomment-557797525>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAGQZM3AKV74KHM7ZW3O6KDQVEU3VANCNFSM4JQXCY5Q>.

The minimum ICU version for Node.js must be at least the minimum ICU version for V8. PR-URL: nodejs#30608 Refs: nodejs#30607 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

srl295 · 2019-11-26T19:32:46Z

OK rebased and cleaned up. PTAL

MylesBorins

LGTM

MylesBorins · 2019-11-27T06:14:50Z

Looks like there are some linting errors

https://travis-ci.com/nodejs/node/jobs/260718954#L301-L305

- update v8 guide to mention ICU - move content from the tools/icu/README.md but leave a pointer Fixes: nodejs#26108 Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> PR-URL: nodejs#30607

MylesBorins · 2019-11-27T06:20:31Z

I've pushed a fix to the lint. Will land if Travis is Green

- update v8 guide to mention ICU - move content from the tools/icu/README.md but leave a pointer Fixes: #26108 Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> PR-URL: #30607 Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins · 2019-11-27T06:55:29Z

landed in f36331c

The minimum ICU version for Node.js must be at least the minimum ICU version for V8. PR-URL: #30608 Refs: #30607 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

- update v8 guide to mention ICU - move content from the tools/icu/README.md but leave a pointer Fixes: #26108 Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> PR-URL: #30607 Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

The minimum ICU version for Node.js must be at least the minimum ICU version for V8. PR-URL: #30608 Refs: #30607 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

srl295 added doc Issues and PRs related to the documentations. i18n-api Issues and PRs related to the i18n implementation. labels Nov 22, 2019

srl295 requested review from Trott and refack November 22, 2019 23:19

srl295 self-assigned this Nov 22, 2019

nodejs-github-bot added tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Nov 22, 2019

srl295 requested a review from ryzokuken November 22, 2019 23:19

ryzokuken approved these changes Nov 22, 2019
View reviewed changes

srl295 mentioned this pull request Nov 22, 2019
tools: Update icu to 65.1 #30232
Closed
4 tasks

srl295 mentioned this pull request Nov 23, 2019
doc: document ICU time zone data update process #30364
Closed

richardlau mentioned this pull request Nov 23, 2019
test: test for minimum ICU version consistency #30608
Merged
3 tasks

richardlau approved these changes Nov 23, 2019
View reviewed changes

Comment threaddoc/guides/maintaining-icu.md Outdated

vsemozhetbyt reviewed Nov 23, 2019
View reviewed changes

Comment threadtools/icu/README.md Outdated
Comment threaddoc/guides/maintaining-icu.md Outdated
Comment threaddoc/guides/maintaining-icu.md Outdated

srl295 force-pushed the doc-icu-patches branch from af3a103 to 4febbfeCompare November 26, 2019 19:21

MylesBorins approved these changes Nov 27, 2019
View reviewed changes

srl295and others added 2 commits November 27, 2019 01:15

doc,deps: document how to maintain ICU in Node.js
24cd1c5
- update v8 guide to mention ICU - move content from the tools/icu/README.md but leave a pointer Fixes: nodejs#26108 Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> PR-URL: nodejs#30607

fixup: fix lint
5e50f4a

MylesBorins force-pushed the doc-icu-patches branch from 4febbfe to 5e50f4aCompare November 27, 2019 06:19

MylesBorins closed this Nov 27, 2019

srl295 deleted the doc-icu-patches branch November 27, 2019 18:10

BridgeAR mentioned this pull request Dec 3, 2019
v13.3.0 proposal #30774
Merged

targos added dont-land-on-v10.x labels Jan 13, 2020

richardlau mentioned this pull request Jul 11, 2020
[v12.x backport] deps: V8: cherry-pick eec10a2fd8fa #34300
Closed

Uh oh!

Uh oh!

Conversation

srl295 commented Nov 22, 2019

Uh oh!

srl295 commented Nov 22, 2019

Uh oh!

ryzokuken left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Nov 22, 2019

Uh oh!

srl295 commented Nov 22, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srl295 commented Nov 23, 2019

Uh oh!

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

targos commented Nov 23, 2019

Uh oh!

srl295 commented Nov 23, 2019 via email

Uh oh!

srl295 commented Nov 26, 2019

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Nov 27, 2019

Uh oh!

MylesBorins commented Nov 27, 2019

Uh oh!

MylesBorins commented Nov 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

srl295 commented Nov 22, 2019•
edited
Loading