-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
The data are needed for new Intl.NumberFormat options added by V8. Fixes: nodejs#29734
Is it worth adding a regression test? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with caveats:
- this PR does not refresh/regen the canned data so it will still fail.
- however, the data would get updated normally with the 65.1 bump ICU 65.1 #29540
- however, 'small icu' itself is no longer the default with Make full-icu the default #29522
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.
@richardlau i would make a regression test a future item, see commenet aboveee. |
@srl295 I'd be happy to refresh the data if you tell me how to do it! I already executed |
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 |
Landed in 634a9a9 |
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>
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>
The data are needed for new Intl.NumberFormat options added by V8.
Fixes: #29734
@nodejs/intl