-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc: advices to use full-icu package is outdated #36057
Comments
@Mumeii fixing that up sounds good. Any chance you want to submit a pull request ? |
@mhdawson : Hi ! I wouldn't mind submitting a PR if I wasn't still puzzled : In fact, I can't tell :
From our development team point of view, it seems it would make sense if it was also working on full-icu release. Picture the trouble we're facing right now due to that restriction :
We've also thought about another work around, that would necessitate to :
But :
Another "work around" would be to be able to base our CI on some kind of But can't tell if that solution is viable from a docker image management point of view, and if more than an handful of projects would benefits having such images available anyway. Do you have stats 📉 📈 somewhere that would help telling which is the usage ration between both flavor ?
|
cc @nodejs/i18n-api |
|
Not exactly. It should be advising you to use
it could fetch data files for all versions. |
because if it is compiled with full-icu there's nothing else to load. As I mentioned above, |
@srl295 thanks for jumping in with the clarifications. |
@Mumeii soorry I did not analyze your whole situation as much before.
To be clear, This is a misunderstanding. You are not up to date by staying on 12. Quite the opposite. So upgrade to 14 and dont worry ablut full icu anymore. |
@srl295 : Hi !
No troubles. Thanks for your feedbacks and the clarifications 🙏 If I understand you the right way, it mean that it's the documentation that must be updated. Thus, can you help me one last time, in order for me to make a sound documentation PR ? Am I wrong saying that :
Mean that :
Mean that :
Mean that :
Once again :
|
👍
Yes.
Correct. In fact, the way full-icu was currently implemented, if you have ICU 63.1 it would not properly fetch ICU 63.2 data (see closed issue nodejs/full-icu-npm#35 because that specific issue was moot).
This is not correct, So there are other ways to do what full-icu does, but it was intended to be the simplest one.
Correct. There is no compatibility at build time for any other major version.
ICU almost never releases even minor version changes of the data. (There is a whole separate process around updating time zone data but I will exclude that for now.) I can not remember a case where. would recommend a data update separate from the code update. So I don't think the scenario 'get the latest official ICU data' really happens in practice, besides upgrading from, say, ICU 67 to ICU 68.
No. It means:
You are very welcome. As of mid- year this is suddenly not part of my paid job at all, so it would be great to have others help with updating the documentation! |
@srl295 :
Ok, so from a Node In that case I guess the simplest way having our project staying up to date with the i18n data would be to move from a, let's say, Good to know. About the 2., it was a cumbersome way for me to mention that :
It always feel like a drawback to me when I can't get this That's why I feel stuck between not been eager to deal with a compilation, and still wanting to be able to choose the level of i18n data pre-package integration. Having those extra versions built and released as Docker hub official image would be great (at least for me 👅), but I guess that it would be hell of a mess to build, to tag, and to manage on the long run. |
@Mumeii What we do in Fedora, we always build with IMHO, the ideal case would be for the official Node container images to follow this same pattern; build with small-icu and provide a simple mechanism (such as a bash script) to the data directory. |
@sgallagher (hi!) yes, either use the full icu in the image, or do as you said. |
The version specified in this issue has reached EoL. If this is no longer relevent, feel free to self-close. |
@RedYetiDev was this changed in later versions of the doc as it says version 13 and above. If you checked an it's fixed in recent versions then I agree it can be closed, otherwise it would still apply. |
It looks fixed to me, but given the conversation around it, I wanted confirmation before closing the issue |
If current docs are fine then close it. People DO still build and use small-icu with custom builds. |
📗 API Reference Docs Problem
Location
Affected URL(s):
Description
Since the first version delivered with full-icu by default, Node 13.x.x, the documentation keep advising to use full-icu package if we want to keep up to date with the latest i18n data.
But unfortunately, this package only fetch ICU data files for node versions built with small-icu support ...
See the corresponding piece of code here
Thus, it is miss leading the reader to be advised to use a tool which is in mutual exclusion with the system that is released by default.
Obviously, not all users of Node 13 and above will go for the default full-icu one, especially if they have memory footprint constraints.
But I guess that with the whole [ micro services / containerization / cloud ] hype those days, most peoples don't care much to finely tune there images and go for the official ones on Docker hub ... which themselves comes in a single flavor each, that is, the full one since Node 13 🍭
submit a pull request.
The text was updated successfully, but these errors were encountered: