Skip to content
This repository has been archived by the owner on Dec 5, 2019. It is now read-only.

fix: disable inline optimization by default #308

Merged
merged 1 commit into from Jun 15, 2018

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Jun 9, 2018

fixes #307 fixes #264

Also we have problem with using option as object, they rewrited after first run.
Also improve readme
A lot of new tests

output: {
shebang: true,
comments: false,
beautify: false,
semicolons: true,
...output,
},
// Ignoring sourcemap from options
// Ignoring sourceMap from options
Copy link
Member Author

Choose a reason for hiding this comment

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

Typo in option name

...parse,
},
compress: {
...compress,
Copy link

Choose a reason for hiding this comment

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

this won't handle compress: false

Copy link

Choose a reason for hiding this comment

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

nor will it handle compress: true

...compress,
},
mangle: mangle == null ? true : {
...mangle,
Copy link

Choose a reason for hiding this comment

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

this won't handle mangle: false

Copy link

Choose a reason for hiding this comment

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

nor will it handle mangle: true

src/index.js Outdated
@@ -42,6 +42,9 @@ class UglifyJsPlugin {
include,
exclude,
uglifyOptions: {
compress: {
inline: false,
Copy link

@kzc kzc Jun 9, 2018

Choose a reason for hiding this comment

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

inline: 1 has no reported issues in uglify-es@3.3.9.

$ echo '!function(){ console.log(42); }();' | bin/uglifyjs -c inline=false
!function(){console.log(42)}();
$ echo '!function(){ console.log(42); }();' | bin/uglifyjs -c inline=1
console.log(42);

@kzc
Copy link

kzc commented Jun 9, 2018

If this plugin is upgraded to use terser in the future (PR #296), then the inline override can be reverted.

@alexander-akait
Copy link
Member Author

@kzc we can not merge use terser as patch/minor release, it is require major release.
How we can solve this:

  1. Disable inline for 1 version (patch release).
  2. Allow people using own cache keys feature: add support for additional cache keys option #304 (need implement function support) (minor release).
  3. Implement feature using own minify function (minor release).
  4. Documented how plugin can setup using uglify-js and terser (patch release).
  5. Switch on terser for next major version (major release).

@kzc
Copy link

kzc commented Jun 15, 2018

we can not merge use terser as patch/minor release, it is require major release

I disagree. terser is a drop in replacement for uglify-es. It's a minor change.

Disabling inline in uglify-es only addresses a couple of bugs. And it could introduce other bugs in this uglify wrapper plugin as per my PR review comments above.

@alexander-akait
Copy link
Member Author

@kzc other developers disagree with

I disagree. terser is a drop in replacement for uglify-es. It's a minor change.

All I'm trying to do is find the best solution for everyone and I'm tired of being between these disputes. You suggest using minor release (only relying on your words) it's too dangerous and better use major release, it is allow people safely upgrade uglify plugin (If the update introduces new bugs they can roll back without fixing the version in package.json).

@kzc
Copy link

kzc commented Jun 15, 2018

I've been involved with the development of both uglify-es and terser since their inception. Terser is just a minor fork of uglify-es with a name change with dozens of bug fixes. But if you're more comfortable with a major for that, so be it. webpack@4 users would have to explicitly load the new major plugin to get the bug fixes.

@alexander-akait
Copy link
Member Author

alexander-akait commented Jun 15, 2018

@kzc After release major version with terser we wait some times to ensure all work as expected, when bump this in webpack@4

@shellscape
Copy link

LGTM

@alexander-akait alexander-akait merged commit 6ab0918 into master Jun 15, 2018
@alexander-akait alexander-akait deleted the fix-disable-inline-optimization-by-default branch June 15, 2018 18:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants