Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: enable unit data in small-icu #29735

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Sep 27, 2019

The data are needed for new Intl.NumberFormat options added by V8.

Fixes: #29734

$ ./node                                                                                                                                          
Welcome to Node.js v13.0.0-pre.
Type ".help" for more information.
> new Intl.DateTimeFormat('es', {month: 'long'}).format(new Date(9E8));
'January'
> Intl.NumberFormat('en', {style: 'unit', unit: 'meter-per-second', notation: 'scientific'}).format(299792458)
'2.998E8 m/s'

@nodejs/intl

The data are needed for new Intl.NumberFormat options added by V8.

Fixes: nodejs#29734
@targos targos added i18n-api Issues and PRs related to the i18n implementation. dont-land-on-v8.x labels Sep 27, 2019
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Sep 27, 2019
@richardlau
Copy link
Member

Is it worth adding a regression test?

Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

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

LGTM with caveats:

so a unit test in this pr as is will not pass. But it may be worth putting this PR in (with no test until later) so that next time small icu data is updated, or manually built post-#29522 (because it will remain an option), units are included.

@srl295
Copy link
Member

srl295 commented Sep 27, 2019

@richardlau i would make a regression test a future item, see commenet aboveee.

@targos
Copy link
Member Author

targos commented Sep 27, 2019

@srl295 I'd be happy to refresh the data if you tell me how to do it! I already executed python tools/icu/shrink-icu-src.py to update deps/icu-small/source/data/in/icudt64l.dat

@srl295
Copy link
Member

srl295 commented Sep 27, 2019

@srl295 I'd be happy to refresh the data if you tell me how to do it! I already executed python tools/icu/shrink-icu-src.py to update deps/icu-small/source/data/in/icudt64l.dat

OK, too quick on the review… i didn't notice the binary file (of course there's nothing per se to review in the UI). Sounds like you did it the right way.

However, as I noted, both #29540 and #29522 will obviate the change in the repo. I'm hoping to land #29522 soon.

… BUT, this does affect (i'm assuming) shipping v12. It's "only" a 3mb data file churn. So maybe it's fine to go ahead and land this in master so that it can be backported to v12

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 30, 2019
@Trott
Copy link
Member

Trott commented Sep 30, 2019

Landed in 634a9a9

@Trott Trott closed this Sep 30, 2019
Trott pushed a commit that referenced this pull request Sep 30, 2019
The data are needed for new Intl.NumberFormat options added by V8.

Fixes: #29734

PR-URL: #29735
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@targos targos deleted the small-icu-unit branch September 30, 2019 14:05
@srl295 srl295 mentioned this pull request Sep 30, 2019
4 tasks
targos added a commit that referenced this pull request Oct 1, 2019
The data are needed for new Intl.NumberFormat options added by V8.

Fixes: #29734

PR-URL: #29735
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
targos added a commit that referenced this pull request Oct 1, 2019
Notable changes:

* build:
  * This release fixes a regression that prevented from building Node.js
    using the official source tarball.
    #29712
* deps:
  * Updated small-icu data to support "unit" style in the
    `Intl.NumberFormat` API.
    #29735

PR-URL: #29796
targos added a commit that referenced this pull request Oct 1, 2019
Notable changes:

* build:
  * This release fixes a regression that prevented from building Node.js
    using the official source tarball.
    #29712
* deps:
  * Updated small-icu data to support "unit" style in the
    `Intl.NumberFormat` API.
    #29735

PR-URL: #29796
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node 12.11.0: Scientific & Engineering notation NumberFormat bug
8 participants