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 compile stats in multiple packaging #278

Merged
merged 3 commits into from Nov 15, 2017

Conversation

Projects
None yet
3 participants
@dangcuuson
Copy link
Contributor

dangcuuson commented Nov 15, 2017

What did you implement:

Fix a problem of #261

How did you implement it:

Extract stats options of first webpack item

How can we verify it:

The old code will fail with the updated unit test
test-before-change

The new code doesn't
screenshot from 2017-11-15 23-52-16

Todos:

  • Write tests
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

}

const compileOutputPaths = [];
const consoleStats = this.webpackConfig.stats || {

This comment has been minimized.

@HyperBrain

HyperBrain Nov 15, 2017

Member

Wouldn't just

const consoleStats = this.webpackConfig.stats || _.get(this, 'webpackConfig[0].stats') || { ...

be enough instead of the ifs below? Then it would try plain, then array and finally fall back to the defaults.

@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Nov 15, 2017

@dangcuuson With the one liner above the unit test changes will be minimal too (just one test case). With the big if solution, you have several new code branches that you need to test (even the stats that you initialize now in there).

@dangcuuson dangcuuson force-pushed the dangcuuson:fix-compile-stats branch from 3afbf1e to 986f15d Nov 15, 2017

const testWebpackConfig = {
stats: 'minimal'
};
let mockStats = {

This comment has been minimized.

@HyperBrain

HyperBrain Nov 15, 2017

Member

This can be const

This comment has been minimized.

@dangcuuson

dangcuuson Nov 15, 2017

Author Contributor

fixed!

return (expect(module.compile()).to.be.fulfilled);
})
.then(() => {
expect(mockStats.toString.args).to.eql([[testWebpackConfig.stats], [testWebpackConfig.stats]]);

This comment has been minimized.

@HyperBrain

HyperBrain Nov 15, 2017

Member

You can do the same for "mockStats.toString.firstCall.args" to check that the plain webpack config's stats was used there. Or is that already covered with this check?

This comment has been minimized.

@dangcuuson

dangcuuson Nov 15, 2017

Author Contributor

It already covered in that checks since sinon.args will list the history of all args passed to the spy function, but I have added the first call check. Not sure if excessive test is good or bad 😈

@dangcuuson dangcuuson force-pushed the dangcuuson:fix-compile-stats branch from 0421dd6 to e2eaa64 Nov 15, 2017

@dangcuuson dangcuuson force-pushed the dangcuuson:fix-compile-stats branch from e2eaa64 to f5b13e4 Nov 15, 2017

@HyperBrain
Copy link
Member

HyperBrain left a comment

I did a local check with your branch with a bigger project (packaged individually) and it work perfectly 👍

With stats: 'minimal' i got

Serverless: Bundling with Webpack...
   185 modules
   134 modules
   170 modules
   130 modules
   130 modules
   131 modules
   166 modules
   132 modules
   131 modules
   130 modules
   189 modules
   3 modules

@HyperBrain HyperBrain added this to the 4.1.0 milestone Nov 15, 2017

@dangcuuson

This comment has been minimized.

Copy link
Contributor Author

dangcuuson commented Nov 15, 2017

That's good! 👍

@HyperBrain HyperBrain merged commit 727f055 into serverless-heaven:master Nov 15, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 96.48%
Details
@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Nov 21, 2017

Released with 4.1.0

@tommedema

This comment has been minimized.

Copy link

tommedema commented Dec 3, 2017

@HyperBrain small remaining issue is that this is causing a lot of whitespace:

Serverless: Bundling with Webpack...






Serverless: Package lock found - Using locked versions

With webpack.config.js having stats: 'errors-only' defined.

@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Dec 3, 2017

@tommedema Thanks for the hint 💯 . It might be that 'errors-only' emits an empty console log when printing the stats. Maybe that can be checked for this setting and the log can be skipped. Can you open an issue for that, so that it is tracked?

@tommedema

This comment has been minimized.

Copy link

tommedema commented Dec 3, 2017

Thanks, I will create an issue in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment