Skip to content
New issue

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

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

Already on GitHub? # to your account

intl: Will Intl.Segmenter need full ICU in Node.js even for the 'en' locale? #24039

Closed
vsemozhetbyt opened this issue Nov 2, 2018 · 13 comments
Closed
Labels
i18n-api Issues and PRs related to the i18n implementation. v8 engine Issues and PRs related to the V8 dependency.

Comments

@vsemozhetbyt
Copy link
Contributor

See tc39/proposal-intl-segmenter#50

@vsemozhetbyt
Copy link
Contributor Author

cc @nodejs/v8 @nodejs/intl.

@vsemozhetbyt vsemozhetbyt added v8 engine Issues and PRs related to the V8 dependency. i18n-api Issues and PRs related to the i18n implementation. labels Nov 2, 2018
@devsnek
Copy link
Member

devsnek commented Nov 2, 2018

I'm a fan of anything that forces us to ship full icu

@vsemozhetbyt
Copy link
Contributor Author

The answer from the implementers:

Question: Will this need full ICU in Node.js even for the 'en' locale?
Answer: YES

Well, this is a strong argument to ship full ICU by default.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Nov 2, 2018

Our table is not correct anymore: small-icu for Intl is not even partial (English-only) now.

@jasnell
Copy link
Member

jasnell commented Nov 2, 2018

/cc @srl295

@refack
Copy link
Contributor

refack commented Nov 2, 2018

Well, this is a strong argument to ship full ICU by default.

As I mentioned in #21676, we should separate dev & test builds from release builds.
Turning on full-icu only for release builds has IMO the most benefit, while carries the least cost for the current dev & test workflow (and build infra overhead).
We could add some full-icu specific tests that will run only as part of the daily CI job.

/CC @nodejs/testing

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Nov 2, 2018

For this particular issue, see this and this comment:

If they cut out necessary icu data for such configuration there are no way v8 code can load the icu data they cut out. that should be a node.js bug instead. - They cut out too much!

@Trott
Copy link
Member

Trott commented Nov 6, 2018

Is there anything to do at this time other than be aware that This Is Coming?

@Trott
Copy link
Member

Trott commented Nov 10, 2018

Should this be closed for the time being? If not, what is the condition for closing?

@vsemozhetbyt
Copy link
Contributor Author

what is the condition for closing?

Maybe clarification if this is wontfix from the ICU team?

@Trott
Copy link
Member

Trott commented Nov 19, 2018

@nodejs/intl Are we going to start shipping full icu? Or not? Or to be determined?

@srl295
Copy link
Member

srl295 commented Dec 18, 2018

So,for this case, I'd think we would just change the definition of 'small icu' to include the break iterator files.

It's this line, to change brkiter from none to proabbly locales (meaning just 'en' by default) https://github.com/nodejs/node/blob/master/tools/icu/icu_small.json#L19

So probably this issue should be closed with "no", or used to put the needed fix in once this v8 change lands.

@richardlau
Copy link
Member

We're shipping full icu by default now so closing.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
i18n-api Issues and PRs related to the i18n implementation. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

7 participants