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 ES6 Support #71

Merged
merged 6 commits into from Nov 27, 2019
Merged

Add ES6 Support #71

merged 6 commits into from Nov 27, 2019

Conversation

@SukkaW
Copy link
Member

SukkaW commented Nov 7, 2019

Close #70.
Another filter register is added in order to avoid require minifier every time the filter is executed, which might have less impact on performance.

@SukkaW SukkaW requested review from tomap and curbengh Nov 7, 2019
@SukkaW

This comment has been minimized.

Copy link
Member Author

SukkaW commented Nov 7, 2019

I think we shouldn't use terser to completely replace uglify-js. I have noticed the different behaviors when I bring up test for the plugin.
Although terser has ecma: 5 as its default configuration, I still believe terser will compress the code into smaller es6 equivalent forms, which we should avoid.

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Nov 9, 2019

I have noticed the different behaviors when I bring up test for the plugin.

Example? Does the behavior break browser compatibility (i.e. IE)?

@SukkaW

This comment has been minimized.

Copy link
Member Author

SukkaW commented Nov 9, 2019

@curbengh

This is the result of uglify;

result.should.eql('var x={baz_:0,foo_:1,calc:function(){return this.foo_+this.baz_},bar_:2,baz_:3};console.log(x.calc());');

And this is the result of terser:

result.should.eql('var x={baz_:(0,3),foo_:1,calc:function(){return this.foo_+this.baz_},bar_:2};console.log(x.calc());');

// UglifyJS
var x={baz_:0,foo_:1,calc:function(){return this.foo_+this.baz_},bar_:2,baz_:3};console.log(x.calc());
// Terser
var x={baz_:(0,3),foo_:1,calc:function(){return this.foo_+this.baz_},bar_:2};console.log(x.calc());

Although it is pretty much equivalent, but it still raises a question if terser would have any other different behaviors.

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Nov 9, 2019

It looks like Terser compresses better, which is expected since it's more advanced than UglifyJS, so they would produce different output as a result.

I suggest to drop uglify in the next major (i.e. when dropping Node 8).

Copy link
Contributor

curbengh left a comment

doc

index.js Show resolved Hide resolved

const result = uglifyFilter.apply(ctx, [code, { path: 'source/test.js' }]);

result.should.eql('var x={baz_:0,foo_:1,calc:function(){return this.foo_+this.baz_},bar_:2,baz_:3};console.log(x.calc());');

This comment has been minimized.

Copy link
@curbengh

curbengh Nov 9, 2019

Contributor

when hardcoding the result, it could break in newer version of uglify/terser (just like hexojs/hexo#3829). I think comparing String.length should suffice.

This comment has been minimized.

Copy link
@SukkaW

SukkaW Nov 9, 2019

Author Member

If I just use:

const uglify = require('uglify-js');

// Just as an example. The usage of UglifyJS & Terser shouldn't be uglify(code).
uglifyFilter.apply(ctx, [code, { path: 'source/test.js' }]).should.eql(uglify(code));

Then the unit-test will become useless.

This comment has been minimized.

Copy link
@curbengh

curbengh Nov 9, 2019

Contributor

what I mean is result.length.should.below(code.length).

This comment has been minimized.

Copy link
@curbengh

curbengh Nov 9, 2019

Contributor

Even uglifyFilter.apply(ctx, [code, { path: 'source/test.js' }]).should.eql(uglify(code)); is useful

For example, try parsing config.uglify.priority without delete jsOptions.priority, the unit test would fail. In this case, the unit test correctly identify that the plugin wouldn't work if delete jsOptions.priority is missing.


Another example is parsing the options

uglifyFilter.apply(ctx, [code, { path: 'source/test.js' }]).should.eql(uglify(code, options));

in this case, the unit test can check whether the plugin parse custom options correctly.

This comment has been minimized.

Copy link
@SukkaW

SukkaW Nov 9, 2019

Author Member

what I mean is result.length.should.below(code.length).

That sounds as a good idea!

This comment has been minimized.

Copy link
@SukkaW

SukkaW Nov 9, 2019

Author Member

For example, try parsing config.uglify.priority without delete jsOptions.priority, the unit test would fail. In this case, the unit test correctly identify that the plugin wouldn't work if delete jsOptions.priority is missing.

If something like delete jsOptions.priority is missing, current unit test will not passed as well because when no valid configuration is passed to UglifyJS / Terser will resulted in undefined.

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Nov 9, 2019

Except for the documentation and unit test, I already tested locally, WFM.

README.md Outdated
- **exclude**: Exclude files. Use [glob expressions](https://github.com/micromatch/micromatch#extended-globbing) for path matching.
- **es6**: Enable es6 compression. Default is `false`
- When enabled, hexo-uglify will use [terser](https://github.com/terser/terser) as compressor, otherwise [uglifyjs](https://github.com/mishoo/UglifyJS2) will be used.
- The API & minify options of terser is fully compatible with uglifyjs.

This comment has been minimized.

Copy link
@curbengh

curbengh Nov 9, 2019

Contributor

It's not fully compatible, e.g. Terser has Safari compatibility mode.

@SukkaW SukkaW requested a review from curbengh Nov 9, 2019
@SukkaW SukkaW merged commit 083ec03 into hexojs:master Nov 27, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
coverage/coveralls First build on es6 at 95.0%
Details
@SukkaW SukkaW deleted the SukkaW:es6 branch Nov 27, 2019
@curbengh curbengh mentioned this pull request Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.