[Audit][Implementation] - Remove Intl Polyfills which are supported by Hermes by separating iOS and android implementation #38565
Conversation
hurali97
changed the title
|
Please add #35234 to the Fixed issues |
This was discovered as part of the audit phase, see here. Once discovered, we discussed it here and then proceeded with the implementation. We were aware that some of the Polyfill APIs are not supported by Hermes and that's why we didn't remove them. Regarding the As you've mentioned for |
| require('@formatjs/intl-locale/polyfill'); | ||
|
|
||
| // Required to polyfill NumberFormat on iOS | ||
| // see: https://github.com/facebook/hermes/issues/1172#issuecomment-1776156538 | ||
| require('@formatjs/intl-pluralrules/polyfill'); | ||
| polyfillNumberFormat(); |
There was a problem hiding this comment.
There is an additional check inside that function: hasOldCurrencyData. Seems like even if the API is supported it may not work as expected e.g. old API. Seeing that we have an early return in enabling that polyfill it may be safer to keep it
There was a problem hiding this comment.
@s77rt I have some reservations around hasOldCurrencyData, if you could explain a bit more about it. So the thing is the output of this function differs only by a space character, otherwise both of the strings are the same on all platforms.
I printed both values of the comparison in the console and it appears that the output by NumberFormat contains a special space character which is different from normal space character.
If I update this const value, only then we do early return otherwise we are always polyfilling NumberFormat. Android and Web also works fine BUT iOS doesn't. Which means that the below code is not working as expected. Ideally, iOS should not fall in the early-return. We probably want to update this condition to include Intl.NumberFormat(locale, options).formatToParts(number) because this is what's missing in iOS and it causes the crash.
/**
* Check if the locale data is as expected on the device.
* Ensures that the currency data is consistent across devices.
*/
function hasOldCurrencyData(): boolean {
return (
new Intl.NumberFormat(CONST.LOCALES.DEFAULT, {
style: CONST.POLYFILL_TEST.STYLE,
currency: CONST.POLYFILL_TEST.CURRENCY,
currencyDisplay: CONST.POLYFILL_TEST.FORMAT,
}).format(Number(CONST.POLYFILL_TEST.SAMPLE_INPUT)) !== CONST.POLYFILL_TEST.EXPECTED_OUTPUT
);
}
export default function () {
if (Intl && 'NumberFormat' in Intl && !hasOldCurrencyData()) {
return;
}
require('@formatjs/intl-numberformat/polyfill-force');
// Load en & es Locale data
require('@formatjs/intl-numberformat/locale-data/en');
require('@formatjs/intl-numberformat/locale-data/es');
}
There was a problem hiding this comment.
iOS do not fall into the early-return, perhaps you tested after the polyfill was applied already.
Got same results on Android
There was a problem hiding this comment.
@s77rt Did you test with the applied diff? I also get the same results as you but without the diff which is what we have in main.
With the diff, each platform falls into the early return and only iOS fails because polyfill isn't applied and we get error on Intl.NumberFormat(locale, options).formatToParts(number). So I am wondering whether we should include formatToParts in the condition for early return?
Otherwise, let's keep the things as is 👍
There was a problem hiding this comment.
@hurali97 Let's do the following:
- Fix the
POLYFILL_TEST.EXPECTED_OUTPUTvalue (use correct whitespace) so the polyfill is applied only when needed - Add
formatToPartsto the condition
Summary:
- Intl.NumberFormat polyfill is not needed on Android.
- Intl.NumberFormat polyfill is needed on iOS due to missing formatToParts funcition
- Intl.NumberFormat polyfill is needed on Web due to outdated currency data on some browsers (issue)
There was a problem hiding this comment.
@s77rt Perfect. I have addressed these changes, you can re-review 👍
Reviewer Checklist
Screenshots/VideosAndroid: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
|
|
Thank you! |
|
@luacmartins looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
Tests were passing |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Details
This PR fixes the linked regression issues which were caused by the PR.
Fixed Issues
$ #38447
$ #38504
$ #38502
PROPOSAL: #35234 (comment)
Tests
Test steps for first issue:
Test steps for second issue ( I couldn't verify the second issue as I don't have any Collect workspace, if reviewer could help in testing this case, that'd be great ):
Precondition:
User is an admin of Collect workspace.
Open app
Go to Profile > Workspaces > Collect workspace.
Go to Taxes.
Click Add rate.
Click Value.
Verify you can add tax value
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
N/A
iOS: Native
ios.mov
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
N/A
MacOS: Desktop
N/A