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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: module assets is in buildInfo #6640

Merged
merged 4 commits into from
Mar 5, 2018
Merged

Conversation

clarkdo
Copy link
Contributor

@clarkdo clarkdo commented Mar 1, 2018

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?

Summary

module.assets should be module.buildInfo.assets due to #6111

Does this PR introduce a breaking change?

No

Other information

@ooflorent
Copy link
Member

ooflorent commented Mar 1, 2018

Could you add some tests in test/statsCases?

@clarkdo
Copy link
Contributor Author

clarkdo commented Mar 2, 2018

@ooflorent I will have a try 😄

@webpack-bot
Copy link
Contributor

@clarkdo Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

[1] ./node_modules/a/index.js 17 bytes {0} [built]
chunk {1} main.js (main) 12 bytes >{0}< [entry] [rendered]
[2] ./index.js 12 bytes {1} [built]
[0] ./node_modules/a/1.png 82 bytes {0} [built]
Copy link
Member

Choose a reason for hiding this comment

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

The assets info isn't in the stats string output. It's only in the stats json.

Maybe you can expand this PR a bit and add support for showing emitted files in the Stats.

I could image a small flag like this:

[0] ./node_modules/a/1.png 82 bytes {0} [built] [1 asset]

You also need include a flag moduleAssets to show/hide this information. It should be enabled by default in json output but disabled by default for toString, so it's backward-compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sokra
Sure, I will try to work on int.

@clarkdo
Copy link
Contributor Author

clarkdo commented Mar 4, 2018

@sokra Please review the my changes 😸.

@clarkdo clarkdo force-pushed the module-assets branch 2 times, most recently from 5a6a98d to 957b62c Compare March 4, 2018 05:24
Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

Looks good

lib/Stats.js Outdated
@@ -867,6 +873,9 @@ class Stats {
if (module.built) {
colors.green(" [built]");
}
if (module.assets && module.assets.length) {
colors.magenta(` [${module.assets.length} asset]`);
Copy link
Member

Choose a reason for hiding this comment

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

for module.assets.length > 1 it should be assets instead of asset

Copy link
Contributor Author

@clarkdo clarkdo Mar 5, 2018

Choose a reason for hiding this comment

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

@sokra
Thank you for the review, I have changed it.

I think it's better to extract module.assets.length into a variable such as const assetsSize = module.assets.length, but for keeping the same behavior with other sources(module.warnings, module.errors), I directly used module.assets.length.

@clarkdo
Copy link
Contributor Author

clarkdo commented Mar 5, 2018

@sokra
The travis node-6 building error should not be related to this pr.

And about the PR Quality Review, codes have been prettied by pretty-files and checked by eslint, if I change as Codacy showed, it will be failed in eslint.

@webpack-bot
Copy link
Contributor

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

@sokra sokra merged commit ae2ae4e into webpack:master Mar 5, 2018
@sokra
Copy link
Member

sokra commented Mar 5, 2018

Thanks

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

4 participants