Skip to content

Switch Rollup minifier plugin to rollup-plugin-terser#4203

Merged
daviwil merged 8 commits intoAzure:masterfrom
daviwil:dont-uglify-me-bro
Jul 12, 2019
Merged

Switch Rollup minifier plugin to rollup-plugin-terser#4203
daviwil merged 8 commits intoAzure:masterfrom
daviwil:dont-uglify-me-bro

Conversation

@daviwil
Copy link
Contributor

@daviwil daviwil commented Jul 8, 2019

In PR #4165, @bterlson recommended that I use rollup-plugin-terser instead of rollup-plugin-uglify to mitigate an issue where an ES6 class definition caused the minifier to throw an error about an unexpected token. @HarshaNalluru asked whether we should be using this everywhere, so this PR is an attempt to get us moved over to rollup-plugin-terser anywhere that rollup-plugin-uglify is being used. The keyvault libraries and core-http both use uglifyjs directly (they probably shouldn't) so I didn't update those at this time (but I can if folks feel strongly about it).

I've made changes for each library in individual commits so it's easy to drop any that we don't feel should be moved over!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noticed that core-auth didn't have the tree-shaking test fix that was introduced in #3997 so I added it here so that tests will run correctly.

@HarshaNalluru HarshaNalluru requested a review from XiaoningLiu July 8, 2019 21:10
@bterlson
Copy link
Member

bterlson commented Jul 8, 2019

LGTM!

For history: I would have definitely adopted terser back when I was working on Storage a year ago, but it was riddled with bugs. Since we required the es5 target, terser really wasn't needed. Now that we target es6, it is, so I'm glad things seem to be working much better now!

Copy link
Member

@XiaoningLiu XiaoningLiu left a comment

Choose a reason for hiding this comment

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

@bterlson Storage still targets es5, which bugs you mentioned by changing to terser?

As the promise to support IE in current V10 GA storage SDKs, changing to es6 seems not appliable.

@daviwil daviwil force-pushed the dont-uglify-me-bro branch from 2942c94 to c23025a Compare July 12, 2019 00:10
@daviwil daviwil force-pushed the dont-uglify-me-bro branch from c23025a to 335caa5 Compare July 12, 2019 17:48
@daviwil
Copy link
Contributor Author

daviwil commented Jul 12, 2019

Since tests are passing, I think that the terser-derived output must be working fine. I'll go ahead and merge this now so that all of our packages are configured consistently. If we notice any issues with specific packages we can revert them individually to use rollup-plugin-uglify.

@daviwil daviwil merged commit 2009f05 into Azure:master Jul 12, 2019
@daviwil daviwil deleted the dont-uglify-me-bro branch July 12, 2019 18:37
@bterlson
Copy link
Member

@XiaoningLiu I don't recall anything affecting ES5 patterns so I think this upgrade is fine.

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.

4 participants