Closed BenIsenstein closed 11 months ago
Patch coverage: 100.00%
and project coverage change: +0.13%
:tada:
Comparison is base (
4203265
) 95.66% compared to head (c666cf4
) 95.80%.:exclamation: Current head c666cf4 differs from pull request most recent head 00770f7. Consider uploading reports for the commit 00770f7 to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Love when wanted features come with a PR <3 I am off on holiday with just a small mobile screen unsuitable for reviewing code, but hopefully someone else could have a looksie?
I'm on medical leave I'll try to take a look next week and maybe ask Erick from Node to look
As I don't have much context I think it LGTM
Thanks for the interest folks! I'll look into these failing tests.
Hi all, it looks like the issue in the test suite is being caused by a difference between the mochify and node runtimes. Intl.DateTimeFormat.resolvedOptions()
returns a different value between the two. Any suggestions on how to resolve this?
It's not mochify (which is running Chrome headless) vs Node, but specifically Node 14 breaking. You see the tests passing in Node 16 and 18.
If you look at the engine comparisons page for Intl.DateTimeFormat you can see that fractionalSecondDigits
was only added in Node 14.6. Guessing we run a slightly older version.
In any case, we only support Node LTS versions, and Node 14 LTS has reached its EOL, so we can just remove that :)
I have now removed Node 14 from main
. Does not block this and you do not need to rebase to verify, but now you know 👍
Do you deem it ready for final review and merge?
Rebased just to keep things clean 👍 yes, it's ready for final review!
Thanks for the review! Changes:
supportedLocalesOf
method
Purpose (TL;DR) - Mock the global Intl API
Solve the breaking of timer-dependant methods on the Intl global object by including a mocked version
Background (Problem in detail)
Even after calling install(), the Intl.DateTimeFormat() class will currently use the real time of the system, not the time specified by the fake clock.
Affected methods: DateTimeFormat.format(), DateTimeFormat.formatToParts()
My repo demonstrating the problem, using Jest (which uses sinonjs/fake-timers): https://github.com/BenIsenstein/jest_fix_intl_fake_timers
The fix works by following the patterns used to mock the global Date class and mocking the Intl object. A producer function is written to return an Intl object largely identical to the original, except for the DateTimeFormat() class. DateTimeFormat has several methods that are time-sensitive, and depend on the system time when no argument is passed. These are intercepted and made to default to the clock time.
This fake Intl object is provided to the Global object when install() is called.
Line 1176, inside createClock()
and the original Intl object is restored during uninstall().
Line 979 inside hijackMethod() An else-if clause added to handle hijacking Intl. Because Intl isn't a straightforward global function, rather a global object used like a module, it can't be mocked using the default else clause at the end of hijackMethod(). It has its own clause to set the mocked Intl object globally.
I'm fairly confident on implementation, however less confident on the tests. All advice is appreciated!
Ben