Visitar URL original
fix compile stats in multiple packaging by dangcuuson · Pull Request #278 · serverless-heaven/serverless-webpack · GitHub
Skip to content

fix compile stats in multiple packaging - #278

Merged
HyperBrain merged 3 commits into
serverless-heaven:masterfrom
dangcuuson:fix-compile-stats
Nov 15, 2017
Merged

HyperBrain merged 3 commits into
serverless-heaven:masterfrom
dangcuuson:fix-compile-stats

Conversation

@dangcuuson

@dangcuuson dangcuuson commented Nov 15, 2017 •

Copy link
Copy Markdown
Contributor

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

Comment thread lib/compile.js
}

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

HyperBrain commented Nov 15, 2017 •

Copy link
Copy Markdown
Member

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

Comment thread tests/compile.test.js Outdated
const testWebpackConfig = {
stats: 'minimal'
};
let mockStats = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be const

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed!

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

@HyperBrain HyperBrain Nov 15, 2017 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 😈

@HyperBrain HyperBrain left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

That's good! 👍

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

Copy link
Copy Markdown
Member

Released with 4.1.0

@tommedema

Copy link
Copy Markdown

@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

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown

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

jamesmbourne pushed a commit to jamesmbourne/serverless-webpack that referenced this pull request Oct 15, 2019
…stats

fix compile stats in multiple packaging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants