mint-metrics / mojito-js-delivery

🧪 Source-controlled JS split testing framework for building and launching A/B tests.
https://mojito.mx/docs/js-delivery-intro
Other
16 stars 30 forks source link

ES6 code fails linting & minification in build process #43

Closed kingo55 closed 4 years ago

kingo55 commented 4 years ago

Whenever we try to build the container using ES6 code, gulp-uglify throws errors.

E.g. try adding this to shared-code.js:

let something = 42

When you go to build the container, Gulp Uglify throws:

✗ gulp build
[12:36:35] Using gulpfile ~/Documents/github/mojito-js-delivery/gulpfile.js
[12:36:35] Starting 'build'...

internal/streams/legacy.js:57
      throw er; // Unhandled stream error in pipe.
      ^
GulpUglifyError: unable to minify JavaScript
    at createError (/Users/robertkingston/Documents/github/mojito-js-delivery/node_modules/gulp-uglify/lib/create-error.js:5:14)
    at /Users/robertkingston/Documents/github/mojito-js-delivery/node_modules/gulp-uglify/lib/minify.js:56:15
    at DestroyableTransform._transform (/Users/robertkingston/Documents/github/mojito-js-delivery/node_modules/gulp-uglify/composer.js:12:19)
    at DestroyableTransform.Transform._read (/Users/robertkingston/Documents/github/mojito-js-delivery/node_modules/gulp-uglify/node_modules/read
able-stream/lib/_stream_transform.js:184:10)
    at DestroyableTransform.Transform._write (/Users/robertkingston/Documents/github/mojito-js-delivery/node_modules/gulp-uglify/node_modules/rea
dable-stream/lib/_stream_transform.js:172:83)
    at doWrite (/Users/robertkingston/Documents/github/mojito-js-delivery/node_modules/gulp-uglify/node_modules/readable-stream/lib/_stream_writa
ble.js:428:64)
    at writeOrBuffer (/Users/robertkingston/Documents/github/mojito-js-delivery/node_modules/gulp-uglify/node_modules/readable-stream/lib/_stream
_writable.js:417:5)
    at DestroyableTransform.Writable.write (/Users/robertkingston/Documents/github/mojito-js-delivery/node_modules/gulp-uglify/node_modules/reada
ble-stream/lib/_stream_writable.js:334:11)
    at DestroyableTransform.ondata (internal/streams/legacy.js:15:31)
    at DestroyableTransform.emit (events.js:198:13)

I think we should handle this issue and warn the user if we have to use a less favourable minification library.

Know of any good ES6 ones that are nearly as good as this library?

dapperdrop commented 4 years ago

We may need to transpile ES6 back to ES5 via gulp-babel before uglifying, see: https://stackoverflow.com/questions/44958216/how-to-minify-es6-functions-with-gulp-uglify

allmywant commented 4 years ago

I agree with @dapperdrop .

kingo55 commented 4 years ago

@dapperdrop @allmywant - any reason we wouldn't use an ES6 compatible lib for minification/linting, such as Terser?:

https://www.npmjs.com/package/terser

allmywant commented 4 years ago

@kingo55 Just took a look on terser (3.9K stars), it has an ecma option (default: 5) -- Pass 6 or greater to enable compress options that will transform ES5 code into smaller ES6+ equivalent forms. I guess it can transpile ES6 back to ES5 during minification.

I think we can try terser.

kingo55 commented 4 years ago

I remember trying it a long time ago and it didn't minify as well as gulp-uglify did.

Maybe it's better or on-par now with gulp-uglify?

allmywant commented 4 years ago

We can give it a try.

kingo55 commented 4 years ago

@allmywant are you able to help us out with this? There's a lot I don't know about Uglifiers and ES6, that you would know better.

allmywant commented 4 years ago

OK @kingo55 , I will do.

allmywant commented 4 years ago

@kingo55 I've tried Terser but I was wrong. Terser will not transpile ES6 back to ES5 even I set output.ecma to 5, I have confirmed this in the documentation of Terser. For ES6 codes, e.g. let something = 42, Terser won't throw any errors but and it will keep it as it is in the output file.

So, if we want to use ES6 code and keep minificated code compatible to ES5, we have to do transpiling using babel.

kingo55 commented 4 years ago

@allmywant - thanks for checking that out.

I'll be guided by you and @dapperdrop - if you think babel is the way to go here, I think we should get a PR ready for testing.

allmywant commented 4 years ago

@dapperdrop I think we should transpile ES6 back to ES5 before uglifying, what do you think?

dapperdrop commented 4 years ago

@allmywant yep - I think that will probably be the best way forward until sometime in the future when ES6 is the norm across all browsers and/or older browsers don't need support.

allmywant commented 4 years ago

@kingo55 @dapperdrop I've sent a #46 pr. Here are testing results on my end with tfeb repo:

without babel transpiling: Container gzipped size: 19.20K Time to build: 1.2s

with babel transpiling: Container gzipped size: 19.20K Time to build: 2.38s

So, the disadvantage is more time needed to run gulp build , almost double.

dapperdrop commented 4 years ago

Thanks @allmywant

That seems like a reasonable penalty.

kingo55 commented 4 years ago

Thanks @allmywant - better that we don't throw errors than it taking a bit longer to build IMO.