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 `keep_quoted=strict` mode (PR RFC) #304

Closed
syranide opened this Issue Mar 4, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@syranide
Copy link
Contributor

syranide commented Mar 4, 2019

Bug report or Feature request?
Feature request

Information
I made a PR for uglify-js some time back for adding keep_quoted=strict mode which prevents quoted properties from being automatically reserved (which stops unquoted properties of the same name from being mangled). This is much closer to how google-closure-compiler works; it generates less code, more completely obfuscates code and more clearly brings mistakes in code to light. If you mix quoted and unquoted for the same the property with strict-mode, the name mangling will become incorrect and the app will not work correctly, rather than silently fixing it by making any quoted props reserved. It's not as nice, but it's opt-in and for me a more desirable behavior.

The original PR has died because of lack of interest from maintainers and their insistence that such a transform must only involve changes to a certain set of source files even though the current architecture of uglify makes that impossible. IMHO the PR is as clean as it gets without fundamentally rewriting and fixing underlying architectural issues in uglify.

Link to PR: mishoo/UglifyJS2#3158

I don't mind making it compatible with terser and creating a PR for it, but first I would like to know if you are at all interested or if you're unlikely to accept it anyway. Thoughts?

@fabiosantoscode

This comment has been minimized.

Copy link
Collaborator

fabiosantoscode commented Mar 5, 2019

I see why you have to make changes at multiple stages.

I see you didn't change any other tests to make it work, as it's really low impact.

But! I'd rather you change that new "quote" property to "original_quote". It explains what it's for a bit better.

Also, Terser 4 is coming out and there's plenty to do, so don't expect any attention from me on that PR until 4 is out.

@syranide

This comment has been minimized.

Copy link
Contributor Author

syranide commented Mar 5, 2019

But! I'd rather you change that new "quote" property to "original_quote". It explains what it's for a bit better.

@fabiosantoscode Agree 👍

Also, Terser 4 is coming out and there's plenty to do, so don't expect any attention from me on that PR until 4 is out.

It sounds like you're probably OK with the PR then. But I'll wait until you're done with Terser 4 then, and then I'll port it and submit a proper PR.

Thank you!

@syranide

This comment has been minimized.

Copy link
Contributor Author

syranide commented Mar 8, 2019

Closing issue because I put a PR up instead #310

@syranide syranide closed this Mar 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.