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

fix collapse_vars regression in destructuring #2897

Merged
merged 1 commit into from
Feb 8, 2018
Merged

fix collapse_vars regression in destructuring #2897

merged 1 commit into from
Feb 8, 2018

Conversation

kzc
Copy link
Contributor

@kzc kzc commented Feb 8, 2018

fixes #2896

@@ -208,7 +208,7 @@ function run_compress_tests() {
});
return false;
}
if (0 && test.reminify && !reminify(test.options, input_code, input_formatted, test.expect_stdout)) {
if (test.reminify && !reminify(test.options, input_code, input_formatted, test.expect_stdout)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please leave this out - I don't want to redo this for every harmony merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why release uglify-es at all then if every new uglify-js feature renders it unusable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm performing this merge on a best-effort basis, in the belief that it would be usable for the majority of use cases out there.

If this is not the case and requires more effort on my part, I would indeed stop making any future uglify-es releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your work on uglify-es. I appreciate you'd rather concentrate on uglify-js.

A reminify test failure is no different than a regular test failure. Disabling it would just mask future regressions. It would be best not to release uglify-es with known regressions and just let harmony diverge from master.

Copy link
Collaborator

@alexlamsl alexlamsl Feb 8, 2018

Choose a reason for hiding this comment

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

Okay - in that case this is the last PR I would accept for harmony and it shall be frozen from now on.

Copy link

Choose a reason for hiding this comment

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

@alexlamsl any chance we get a quick bugfix release for uglify-es though? This regression hit many of our projects using uglifyjs-webpack-plugin.

@diervo
Copy link

diervo commented Feb 22, 2018

Do you guys have any ETA for landing the npm version with this fix?

@AviVahl
Copy link

AviVahl commented Feb 23, 2018

@diervo I believe this already landed in v3.3.11... and v3.3.12 is out as well with all changes of current master.

@diervo
Copy link

diervo commented Feb 24, 2018

@AviVahl But thats not for the harmony version isn't it? Or am I confused?
The last commit I see in that branch (https://github.com/mishoo/UglifyJS2/commits/harmony) is still on .10

@AviVahl
Copy link

AviVahl commented Feb 24, 2018

He wrote:

Okay - in that case this is the last PR I would accept for harmony and it shall be frozen from now on.

So I'm really not sure anymore, heh.
Such a confusing repository/project/versioning scheme.

uglify-js's "latest" dist-tag currently points to 3.3.12. uglify-es points to 3.3.9.

@alexlamsl
Copy link
Collaborator

This PR fixes a regression in uglify-es@3.3.10, i.e. the bug is not present in uglify-es@3.3.9 which is the current latest on npm.

@kzc
Copy link
Contributor Author

kzc commented Feb 24, 2018

Since 3.3.10 was deprecated various uglify-es bug fixes were lost however:

#2877
#2881

@alexlamsl To reduce confusion please consider adding a note to the top of the harmony README saying that uglify-es is no longer maintained as per your comment above. That way people can choose to fork the project or choose an alternative ES6+ minifier. It would be logical for the webpack project to take over a fork of the uglify-es project as they have more resources.

@alexlamsl
Copy link
Collaborator

@kzc I don't think anyone would be able to see the note, as https://www.npmjs.com/package/uglify-es only shows a copy of README.md from the published module.

@kzc
Copy link
Contributor Author

kzc commented Feb 24, 2018

@alexlamsl So publish 3.3.11 with that just that README addition to the harmony branch.

@alexlamsl
Copy link
Collaborator

Too much error-prone work for zero gain on my part - uglify-es@3.3.9 works, and I'm fed up with all the noise from that branch to care for the time being.

@kzc
Copy link
Contributor Author

kzc commented Feb 24, 2018

No need to merge master into harmony. Just a README addition.

@alexlamsl
Copy link
Collaborator

I understand what you are trying to say - it means I need to checkout harmony-v3.3.9, manually edit the detached HEAD, then npm publish (which updates everything, not just README.md) and hopes that nothing goes wrong.

I'm just not comfortable with that.

@kzc
Copy link
Contributor Author

kzc commented Feb 24, 2018

No need to do any of that - use latest harmony as is. Add one line to the README saying uglify-es is no longer maintained and then publish 3.3.11. The features need not be in sync with uglify-js - it's just a release number. That way the orphaned 3.3.10 ES6 bug fixes in #2897 (comment) would be included as well. Less confusion for everyone because the latest release would match the harmony branch.

@alexlamsl
Copy link
Collaborator

Doing so you are assuming there are no other bugs within uglify-es@3.3.10 or this PR, which I'm equally uncomfortable with.

@kzc
Copy link
Contributor Author

kzc commented Feb 24, 2018

No code is bug free. It's all relative - latest harmony has fewer ES6 bugs than uglify-es@3.3.9.

Then put a note in the issue template saying uglify-es is no longer maintained so people won't waste their time reporting bugs.

@alexlamsl
Copy link
Collaborator

That is a rational viewpoint, and if I've been getting rational responses that would be the best course of action - hell, I might even be interested in keeping uglify-es going.

For the time being, I just don't want anything to do with it, after receiving such goodwill as, and not limited to, creating dedicated repository just to hurl personal insults in hit-and-run style.

I know I am being difficult, and I do apologise for that.

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