Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Deal with undetected timeZone in Stats.js #9799

Merged
merged 2 commits into from
Oct 11, 2019

Conversation

petetnt
Copy link
Contributor

@petetnt petetnt commented Oct 11, 2019

As seen in facebook/create-react-app#7783 (comment) (and GoogleChrome/lighthouse#1056 and others) if for some reasons the timeZone is undetected the locale-methods throw with "RangeError: Unsupported time zone specified undefined"

The issue itself was fixed in NodeJS 12, but affects other NodeJS LTS versions (6, 8, 10)

What kind of change does this PR introduce?

This PR deals with the issue similar to https://github.com/GoogleChrome/lighthouse/pull/1086/files and https://github.com/GoogleChrome/lighthouse/pull/1086/files

Did you add tests for your changes?

This is bit tricky to test this as it needs OS level changes or ENV variable setup at launch but manual steps to test this is

nvm install 10 # install affected version with nvm
nvm use 10 # checkout the node version

then reverse this check so that the locale time strings are run on the basic tests

if (!typeof obj.builtAt === "number") {

then run the basic tests

yarn test:basic

See all the snapshot tests failing 馃憖 and then run the tests again with Etc/Unknown timezone

TZ=Etc/Unknown yarn test:basic

Expected:

  • All the snapshot tests fail again 馃憖

Actual:

    RangeError: Unsupported time zone specified undefined
        at new DateTimeFormat (native)
        at Date.toLocaleDateString (native)

Apply the changes from this branch to confirm that the issue was fixed.

Does this PR introduce a breaking change?

Nope, it just fixes an unfortunate edge case

What needs to be documented once your changes are merged?

No need for doc changes

Signed-off-by: petetnt pete.a.nykanen@gmail.com

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
@webpack-bot
Copy link
Contributor

For maintainers only:

  • This need to be documented (issue in webpack/webpack.js.org will be filed when merged)

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we鈥檒l review the pull request soon.

@webpack-bot
Copy link
Contributor

It looks like this Pull Request doesn't include enough test cases (based on Code Coverage analysis of the PR diff).

A PR need to be covered by tests if you add a new feature (we want to make sure that your feature is working) or if you fix a bug (we want to make sure that we don't run into a regression in future).

@petetnt Please check if this is appliable to your PR and if you can add more test cases.

Read the test readme for details how to write test cases.

@sokra sokra merged commit 16d2628 into webpack:master Oct 11, 2019
@sokra
Copy link
Member

sokra commented Oct 11, 2019

Thanks

@petetnt petetnt deleted the petetnt-stats-fix branch October 11, 2019 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants