Fix full ICU on tests #4673
Merged
Fix full ICU on tests #4673
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why tests on master are broken?
We haven't upgraded the package
full-icuin a while, so with the publication of NodeJS 12.16.2, our version wasn't compatible.Why we use
full-icuto unit test React Admin?I did some speleology, and found that 2017 commit for admin-on-rest 0.7: b7a2c98
At the time,
<DateField />component was introduced to Intl.DateTimeFormat to format date in many languages.This works well on the browser, it also works well on unit tests since this API is supported by NodeJS. But by default NodeJS doesn't support all languages, so we had to add this package to test our implementation.
<NumberField />also uses the Intl API.Is
full-icuobsolete, as explained on this issue of the package repository?Since the version 8, at least, we can build NodeJS including full ICU with the command
--with-intl=full-icu.This would avoid installing another library to test React Admin.
This is even easier with nvm because you can run
nvm install -s 12 --with-intl=full-icuBut we made the choice to ease contributors development, and not force them rebuilding their local NodeJS.
Will this package always be mandatory in the future?
Starting from NodeJS 13.x, NodeJS will be compiled with full-icu by default.
Hence, we might consider removing
full-icufrom the dev dependency when NodeJS 14.x will enter in Long Term Support (LTS) in October 2020.