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

Check minified code for syntax errors (e.g. by trying to construct an AST) #3075

Closed
mgol opened this issue Apr 25, 2016 · 10 comments · Fixed by #4594 or #4598
Closed

Check minified code for syntax errors (e.g. by trying to construct an AST) #3075

mgol opened this issue Apr 25, 2016 · 10 comments · Fixed by #4594 or #4598

Comments

@mgol
Copy link
Member

@mgol mgol commented Apr 25, 2016

* Description*

Enabling strict mode uncovered an UglifyJS issue (mishoo/UglifyJS#1052) where it was creating code with SyntaxErrors. Perhaps we should try to generate the AST of the generated minified code to check if it doesn't contain syntax errors.

@mgol mgol added the Build label Apr 25, 2016
@mgol mgol added this to the 3.0.1 milestone Apr 25, 2016
@markelog
Copy link
Member

@markelog markelog commented Apr 25, 2016

Validate AST? Sounds awfully like a linter to me. I think @gibson042 meant to just check if we can build an AST.

@mgol mgol changed the title Try to validate the AST of the minified code Check minified code for syntax errors (e.g. by trying to construct an AST) Apr 25, 2016
@mgol
Copy link
Member Author

@mgol mgol commented Apr 25, 2016

@markelog updated the title & description.

@markelog
Copy link
Member

@markelog markelog commented Apr 25, 2016

And it also sounds like, that this could be (if proven viable) implemented on uglify side, like self check of sorts which could be run if correct option is passed (since it would reflect on the performance).

@mishoo what do you think?

@mishoo
Copy link

@mishoo mishoo commented Apr 25, 2016

@markelog Not sure I understand what's your question, but I do agree that @mgol reported a valid issue on UglifyJS. We already have a PR that fixes it; I didn't have time to look into it yet, though — please test if possible.

@markelog
Copy link
Member

@markelog markelog commented Apr 25, 2016

Not sure I understand what's your question

We have an idea, which we currently considering to implement in jquery build process - after uglify minifies the source, run it through parser once again and try to build an AST, if AST is builded, then we could be sure that there is no obvious syntax errors.

Yes, we do run minified version through test suite on various set of browsers with browserstack, but because mentioned error was noticed only at that point, we almost reached our cap on that service, which is not preferred, proposed approach might caught such errors on early stage if automated.

And yes, most errors will not be revealed at that stage, but it seems some of them will, like mishoo/UglifyJS#1052?

I'm wondering if second parser run could be introduced on uglify side as a self check and overall, if you think this idea worth something.

Other option is to run linter on minified version, with limited amount of enabled rules, but running linter on minified version sounds kinda... or is it?

@kzc
Copy link

@kzc kzc commented Apr 25, 2016

A second uglify parse would not have caught this issue:

"use strict";

!function() {
    if (window) function i() {}
}();

as uglify parses it without error:

"use strict";!function(){if(window)function i(){}}();

A full-fledged linter from a different parsing code base or testing with PhantomJS is your best bet.

@mgol
Copy link
Member Author

@mgol mgol commented May 17, 2016

This issue doesn't seem actionable now, it seems it requires further research. Should we move it to the roadmap?

@markelog
Copy link
Member

@markelog markelog commented May 17, 2016

Should we move it to the roadmap?

Done

@timmywil timmywil removed this from the 3.0.1 milestone Jun 30, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018
mgol added a commit to mgol/jquery that referenced this issue Jan 17, 2020
While we have absolutely no style-related expectations to our minified file,
we do care that it's valid ES 5.1. This is now verified.

Fixes jquerygh-3075
@mgol
Copy link
Member Author

@mgol mgol commented Jan 17, 2020

Re-opening since it's now possible to do such a verification via a plain ESLint run.

@mgol mgol reopened this Jan 17, 2020
@mgol mgol added this to the 3.5.0 milestone Jan 17, 2020
@mgol
Copy link
Member Author

@mgol mgol commented Jan 17, 2020

PR: #4594

mgol added a commit that referenced this issue Jan 21, 2020
While we have absolutely no style-related expectations to our minified file,
we do care that it's valid ES 5.1. This is now verified.

Fixes gh-3075
Closes gh-4594
mgol added a commit to mgol/jquery that referenced this issue Jan 21, 2020
Code in jquerygh-4594 configured ESLint properly but didn't change the Gruntfile.js
config to actually lint the minified file. This has now been fixed.

Fixes jquerygh-3075
Ref jquerygh-4594
mgol added a commit to mgol/jquery that referenced this issue Jan 21, 2020
While we have absolutely no style-related expectations to our minified file,
we do care that it's valid ES 5.1. This is now verified.

Fixes jquerygh-3075
Closes jquerygh-4594
mgol added a commit that referenced this issue Jan 21, 2020
While we have absolutely no style-related expectations to our minified file,
we do care that it's valid ES 5.1. This is now verified.

Also, update grunt-eslint as a newer ESLint version is required to be able
to use "extends" inside of the "overrides" section.

Fixes gh-3075
Closes gh-4594
Ref gh-4598
mgol added a commit to mgol/jquery that referenced this issue Jan 21, 2020
While we have absolutely no style-related expectations to our minified file,
we do care that it's valid ES 5.1. This is now verified.

Fixes jquerygh-3075
Closes jquerygh-4594
mgol added a commit that referenced this issue Jan 27, 2020
While we have absolutely no style-related expectations to our minified file,
we do care that it's valid ES 5.1. This is now verified.

Fixes gh-3075
Ref gh-4594
Closes gh-4598
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
5 participants