Skip to content

SizeFormatHelpers: defend against invalid sizes#6905

Merged
sokra merged 1 commit intowebpack:masterfrom
xtuc:fix-handle-unknown-size
Apr 17, 2018
Merged

SizeFormatHelpers: defend against invalid sizes#6905
sokra merged 1 commit intowebpack:masterfrom
xtuc:fix-handle-unknown-size

Conversation

@xtuc
Copy link
Copy Markdown
Member

@xtuc xtuc commented Mar 29, 2018 •
edited
Loading

What kind of change does this PR introduce?

Avoids having NaN undefined in the chunk size.

before:

Hash: a767d25c0ab9f6137502
Version: webpack 4.3.0
Time: 351ms
Built at: 3/29/2018 2:57:47 PM
                           Asset           Size  Chunks             Chunk Names
                       bundle.js       6.33 KiB    main  [emitted]  main
                     0.bundle.js      462 bytes       0  [emitted]
c0a8be7a5c1867a7212e.module.wasm  NaN undefined       0  [emitted]
                      index.html      182 bytes          [emitted]
Entrypoint main = bundle.js
[./src/index.js] 90 bytes {main} [built]
[./src/test.wasm] 71 bytes {0} [built]
Child html-webpack-plugin for "index.html":
     1 asset
    Entrypoint undefined = index.html
    [../../buildin/module.js] (webpack)/buildin/module.js 497 bytes {0} [built]
        + 2 hidden modules

after:

Hash: 6d167786e870e3758cab
Version: webpack 4.3.0
Time: 335ms
Built at: 2018-3-29 15:14:06
                           Asset          Size  Chunks             Chunk Names
                       bundle.js      8.58 KiB    main  [emitted]  main
                     0.bundle.js     492 bytes       0  [emitted]
c0a8be7a5c1867a7212e.module.wasm  unknown size       0  [emitted]
                      index.html     182 bytes          [emitted]
Entrypoint main = bundle.js
[./src/index.js] 90 bytes {main} [built]
[./src/test.wasm] 71 bytes {0} [built]
Child html-webpack-plugin for "index.html":
     1 asset
    Entrypoint undefined = index.html
    [../../buildin/global.js] (webpack)/buildin/global.js 489 bytes {0} [built]
    [../../buildin/module.js] (webpack)/buildin/module.js 497 bytes {0} [built]
        + 2 hidden modules

and webpack/webpack-sources#38 fixes the wasm unknown size.

Did you add tests for your changes?
yes 👍

If relevant, link to documentation update:

Summary

Does this PR introduce a breaking change?
no 👍

Other information

Janpot
Janpot reviewed Mar 29, 2018
View reviewed changes
Comment thread lib/SizeFormatHelpers.js
const SizeFormatHelpers = exports;

SizeFormatHelpers.formatSize = size => {
if (typeof size !== "number" || Number.isNaN(size) === true) {
Copy link
Copy Markdown
Contributor

@Janpot Janpot Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=== true is redundant.

Copy link
Copy Markdown
Member Author

@xtuc xtuc Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

That's a more explicit check otherwise the engine has to run the ToBoolean coercion.

Copy link
Copy Markdown
Contributor

@Janpot Janpot Mar 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Number.isNaN already returns a boolean. There is no benefit, performance-wise nor readability-wise (granted, readability is subjective but I'd argue it makes it less readable), to add === true behind.
In fact as far as performance micro-optimization goes. I bet if(Number.isNaN(size) === true) is actually slightly slower than if(Number.isNaN(size))

Copy link
Copy Markdown
Member

@montogeek montogeek Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave comparison there, but after searching codebase there is someplaces where it is not used:

webpack/lib/DllPlugin.js

Line 26 in fde0183

if (typeof entry === "object" && !Array.isArray(entry)) {

Copy link
Copy Markdown
Contributor

@Janpot Janpot Mar 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, if (Number.isNaN(size)) mentally parses as "if "size" is NaN then" and if (Number.isNaN(size) === true) parses as "if ("size" is NaN) equals true then". That's why the former is more readable. Airbnb styleguide seems to agree with that, couldn't find any sources that defends the latter.
Also did a little performance benchmark and can confirm that the former is faster.
It's ofcourse only a minor issue and in part a matter of personal preference.

@webpack-bot
Copy link
Copy Markdown
Contributor

webpack-bot commented Mar 29, 2018

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@webpack-bot
Copy link
Copy Markdown
Contributor

webpack-bot commented Mar 29, 2018

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).

@xtuc 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 3f99517 into webpack:master Apr 17, 2018
@sokra
Copy link
Copy Markdown
Member

sokra commented Apr 17, 2018

Thanks

@xtuc xtuc deleted the fix-handle-unknown-size branch April 17, 2018 09:23
@renovate renovate bot mentioned this pull request Apr 17, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@montogeek montogeek montogeek left review comments

@sokra sokra sokra approved these changes

+1 more reviewer

@Janpot Janpot Janpot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

PR: CI-ok PR: reviewed-approved PR: small PR: tests-needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@xtuc @webpack-bot @sokra @montogeek @Janpot