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

Add Terser as minifier #26

Merged
merged 13 commits into from
Jan 10, 2020
Merged

Add Terser as minifier #26

merged 13 commits into from
Jan 10, 2020

Conversation

Swaagie
Copy link
Member

@Swaagie Swaagie commented Jan 8, 2020

Summary

Add support for Terser minification.

  • tests added for separate Config class
  • docs added in README.md
  • JSDoc inline

Changelog

Added.

Test Plan

Unit test added using Terser, ES6 source code and proper wrhs.toml fixture.

@Swaagie Swaagie changed the title Gx 18384 Add Terser as minifier Jan 8, 2020
Copy link

@msluther msluther left a comment

Choose a reason for hiding this comment

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

LGTM so far.

Copy link

@chrisabrams chrisabrams left a comment

Choose a reason for hiding this comment

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

What's the advantage of using fingerprinting for hashing the file name vs using Webpack's hashing?

Copy link

@msluther msluther left a comment

Choose a reason for hiding this comment

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

Overall 👍, requesting comments to make sure we have enough debugging information for if/when minification fails.

minifiers/terser.js Show resolved Hide resolved
// Return on a minification error.
//
if (results.error) {
return done(results.error);
Copy link

Choose a reason for hiding this comment

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

while we're in here, can we make sure we know when minification is starting/done in status events (and probably which minifier is being used)? and make sure that this error from the minifiers get tagged in status events in such a way that we know they originated from minification (and which minfiier).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, but I'm going to consider this out of scope. https://github.com/godaddy/carpenterd-worker/blob/master/builder.js#L2 workers-factory is only a dependency of carpenterd-worker so I assume we most likely need to either do the work there or give the workers-factory access to the writeStream. I don't consider that to be something we want though. Ideally workers-factory emits events which we can listen to somewhere around here https://github.com/godaddy/carpenterd-worker/blob/master/builder.js#L251-L272 Happy to make a ticket for this though.

minifiers/config.js Show resolved Hide resolved
test/config.test.js Outdated Show resolved Hide resolved
test/config.test.js Show resolved Hide resolved
@Swaagie Swaagie merged commit 9a7df17 into master Jan 10, 2020
@Swaagie Swaagie deleted the GX-18384 branch January 10, 2020 16:05
@Swaagie Swaagie restored the GX-18384 branch January 10, 2020 16:05
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.

None yet

4 participants