Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Improvements for Closure Compiler compilation #1710

Closed
wants to merge 36 commits into from
Closed

Improvements for Closure Compiler compilation #1710

wants to merge 36 commits into from

Conversation

benmccann
Copy link
Contributor

Syntax fixes helpful for compiling with Closure Compiler advanced optimizations. Even if Closure Compilation is not eventually achieved or is not a goal, it is helpful static analysis to find problems in the code or documentation.

@ahk
Copy link

ahk commented Jan 9, 2013

I'm interested in the goal of this work (working compilation under closure compiler advanced mode), and I'm curious what more work needs to be done to make this a reality.

If it's more than just a matter of fixing compatibility with the angular codebase – like some greater performance problem, pain point, or philosophical issue – I'd be very curious to hear about it.

@benmccann
Copy link
Contributor Author

@ahk this change fixes all of the compilation errors and about half of the warnings if you run a quick sed script to remove the unrecognized jsdoc tags like @ngdoc

@IgorMinar
Copy link
Contributor

can you modify the rake file to configure the closure compiler to make these warnings break the build?

we are interested in all these changes, but we also should ensure that we are not going to introduce more violations going forward.

@benmccann
Copy link
Contributor Author

@IgorMinar that's a great idea hopefully not too long from now. There's still a bunch of warnings beyond these, so I don't think we could make these breaking changes yet. However, this gets us a good portion of the way there and is a good starting point for people to work from.

@mhevery
Copy link
Contributor

mhevery commented Jan 18, 2013

Thanks for your contribution! In order for us to be able to accept it, we ask you to sign our CLA (contributor's license agreement).

CLA is important for us to be able to avoid legal troubles down the road.

For individuals (a simple click-through form):
http://code.google.com/legal/individual-cla-v1.0.html

For corporations (print, sign and scan+email, fax or mail):
http://code.google.com/legal/corporate-cla-v1.0.html

@benmccann
Copy link
Contributor Author

I've already signed the CLA as a contributor to other Google projects. Let me know if I need to do it again, but I think it shouldn't be necessary.

@mhevery
Copy link
Contributor

mhevery commented Jan 18, 2013

not necessary to sign again. thansk

@ghost ghost assigned IgorMinar Jan 22, 2013
@casio
Copy link

casio commented Jan 29, 2013

+1

@IgorMinar
Copy link
Contributor

sigh.. ok. so what I'm going to do is to just squash all these changes into a single commit and try to apply that to the current master. this will be nasty.

What flags for the closure compiler do you use? This is my current setup:

        --compilation_level SIMPLE_OPTIMIZATIONS \
        --language_in ECMASCRIPT5_STRICT \
        --warning_level VERBOSE \
        --externs lib/externs/json.js \
        --js angular.js \
        --js_output_file angular.min.js

@benmccann
Copy link
Contributor Author

Cool, thanks. I'll try to setup some kind of test or something after it's committed to keep this in a good state and share how to get it to run with ADVANCED_OPTIMIZATIONS.

@IgorMinar
Copy link
Contributor

we are interested in getting to the point where we are using advanced optimizations to build angular.min.js and also make it possible to build angular source with your app into a single binary using advanced optimizations.

can you share info about your setup so that I can crosscheck it with what I'm doing?

@benmccann
Copy link
Contributor Author

It basically works already. I used a version of Angular with this patch applied. Then I used a sed script to strip out all the @ngdoc and other unrecognized tags in the jsdoc comments:

#!/bin/bash
sed -i '/.*@ngdoc.*/d' angular.js
sed -i '/.*@element.*/d' angular.js
sed -i '/.*@eventOf.*/d' angular.js
sed -i '/.*@eventType.*/d' angular.js
sed -i '/.*@methodOf.*/d' angular.js
sed -i '/.*@paramDescription.*/d' angular.js
sed -i '/.*@priority.*/d' angular.js
sed -i '/.*@propertyOf.*/d' angular.js
sed -i '/.*@restrict.*/d' angular.js
sed -i '/.*@scope.*/d' angular.js
sed -i '/.*@usageContent.*/d' angular.js

@IgorMinar
Copy link
Contributor

ok, and then you did what? compiled angular.js as a standalone js or together with your app? also what flags did you use? only the advanced_mode flag or something else as well?

@benmccann
Copy link
Contributor Author

Hmm, maybe this doesn't get you quite as far as I'd remembered. It's been so long now. Looks like this'll let Angular run under Closure Compiler in whitespace mode, but there'll be a few more changes required to let it run with variable renaming. I think the issue is that there's a mix of accessing variables with array notation and dot notation. There's a couple ways to fix that, but it might be a bit tougher to figure out.

@IgorMinar
Copy link
Contributor

Alrighty.. I worked through all of your changes and cherry-picked stuff onto my branch. before your changes closure compiler found 285 warnings, after your changes were applied I'm down to 141. So I still have some work to do, but this helped quite a lot.

I'm going to go ahead and close this PR. instead you can follow #1951

thanks a lot for all the work. if you haven't received our T-shirt yet, please fill out this form: http://goo.gl/075Sj

@IgorMinar IgorMinar closed this Feb 5, 2013
IgorMinar added a commit to IgorMinar/angular.js that referenced this pull request Feb 14, 2013
This change resolves all warnings found by the closure compiler
when verbose mode is turned on and all the other flags remain at defaults.

Part of this commit contains code contributed by Ben McCann via PR angular#1710.
@inuwan
Copy link

inuwan commented Feb 28, 2013

I attempted to compile your AngularJS-closure-warning branch with Google Closure using Simple Optimizations. (I build the branch with "rake package").

Using the following Google Closure options:

"checks":{
    "checkEs5Strict":true,
    "checkTypes":"ERROR",
    "checkVars":"ERROR",
    "constantProperty":"ERROR",
    "visibility":"ERROR",
    "strictModuleDepCheck":"WARNING",
    "checkDebuggerStatement":"WARNING"
},

"define":{
    "goog.DEBUG": true,
    "goog.dom.ASSUME_STANDARDS_MODE":true
},

Google Closure gave me the following errors:

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 59 : 24

JSC_PARSE_ERROR. Parse error. illegal character at ../lib/angular/angular.js line 59 : 36

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 59 : 39

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 60 : 10

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 61 : 10

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 62 : 22

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 63 : 22

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 64 : 22

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 67 : 22

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 69 : 22

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 70 : 17

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 71 : 13

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 72 : 22

JSC_PARSE_ERROR. Parse error. missing ( before function parameters. at ../lib/angular/angular.js line 249 : 9

JSC_PARSE_ERROR. Parse error. missing } after function body at ../lib/angular/angular.js line 249 : 9

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 251 : 0

JSC_PARSE_ERROR. Parse error. missing ; before statement at ../lib/angular/angular.js line 254 : 32

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 256 : 0

JSC_PARSE_ERROR. Parse error. missing ; before statement at ../lib/angular/angular.js line 274 : 16

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 275 : 4

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 5037 : 12

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 5038 : 10

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 5039 : 12

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 5040 : 10

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 8469 : 18

JSC_PARSE_ERROR. Parse error. illegal character at ../lib/angular/angular.js line 8469 : 33

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 8469 : 36

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 8470 : 17

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 8573 : 12

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 8575 : 22

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 8582 : 12

JSC_PARSE_ERROR. Parse error. invalid property id at ../lib/angular/angular.js line 9559 : 8

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9559 : 13

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9560 : 16

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9561 : 16

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9562 : 18

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9563 : 17

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9564 : 18

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9565 : 17

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9566 : 7

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9568 : 15

JSC_PARSE_ERROR. Parse error. missing ; before statement at ../lib/angular/angular.js line 9569 : 23

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9574 : 4

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9575 : 2

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9576 : 0

JSC_PARSE_ERROR. Parse error. missing ; before statement at ../lib/angular/angular.js line 9578 : 28

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9579 : 6

JSC_PARSE_ERROR. Parse error. missing ; before statement at ../lib/angular/angular.js line 9580 : 67

JSC_PARSE_ERROR. Parse error. invalid return at ../lib/angular/angular.js line 9659 : 4

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9660 : 2

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9661 : 0

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 9737 : 15

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 10324 : 17

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 10325 : 16

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 10327 : 28

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 10328 : 28

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 10344 : 15

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 10503 : 12

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 11867 : 20

JSC_PARSE_ERROR. Parse error. identifier is a reserved word at ../lib/angular/angular.js line 11884 : 20

JSC_PARSE_ERROR. Parse error. syntax error at ../lib/angular/angular.js line 15163 : 0

BUILD FAILED: 61 error(s), 0 warning(s)

@benmccann
Copy link
Contributor Author

You should look at this pull request instead:
#1951

@inuwan
Copy link

inuwan commented Feb 28, 2013

Thanks. I actually needed to set the following option on plovr (Closure compiler)

"experimental-compiler-options": {
"languageIn": "ECMASCRIPT5_STRICT"
};

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants