Add compiler warning for dangerous use of this
#5244
Conversation
|
@cramforce @erwinmombay PTAL |
| @@ -68,6 +69,8 @@ protected AmpCommandLineRunner(String[] args) { | |||
| } | |||
| CompilerOptions options = super.createOptions(); | |||
| options.setCollapseProperties(true); | |||
| // Warn on improper uses of `this` that `collapseProperties` assumes. | |||
| options.setCheckGlobalThisLevel(CheckLevel.WARNING); | |||
There was a problem hiding this comment.
Could this instead be added here: https://github.com/ampproject/amphtml/blob/master/build-system/tasks/compile.js#L269
Our config is a bit over the place, but we try to use the flags where available instead of Java.
There was a problem hiding this comment.
Done. Note that this will only run for gulp check-test and will be an error rather than a warning.
dreamofabear
force-pushed
the
global_this_warn
branch
2 times, most recently
from
8f1690c
to
aca5bf6
Compare
|
It seems there is one error now. Could you fix it? |
|
i believe @choumx has a separate PR for the fix here #5243 if you could also TAL a that @cramforce since I don't know if @dvoytenko is out. I tested it out and it works but want to make sure the change is OK with custom-elements v0 |
|
If that fix works (as in renders pages) it would be great. It would rely on the specific transpile, I believe. Does it work? Otherwise this can likely be fixed with a |
|
Yeah it will rely on the transpile. I tested it with babel and it worked, i haven't tested it with closure but looked at the transpiled transformation in http://closure-compiler-debugger.appspot.com/ and was what i was expecting. |
|
There's an issue with the Closure build that I'm investigating. |
|
@choumx same, i just tried it and there's an issue of some sort, i'm resetting to your current HEAD since my copy was out of date |
dreamofabear
force-pushed
the
global_this_warn
branch
2 times, most recently
from
2fc358c
to
aacd43e
Compare
|
LGTM |
|
@cramforce Friendly ping. I'm planning to address the refactor of |
|
Sorry, for the late response. No need to always wait for me :) |
dreamofabear
force-pushed
the
global_this_warn
branch
from
53d68cc
to
b84d10d
Compare
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
dreamofabear
force-pushed
the
global_this_warn
branch
from
b84d10d
to
6d6b909
Compare
|
CLAs look good, thanks! |
* add checkGlobalThisLevel(warn) to compiler opts * malte's pr comment
* add checkGlobalThisLevel(warn) to compiler opts * malte's pr comment
Fixes #5229.
From the Closure documentation for
checkGlobalThisLevel:Note: #5243 needs to land first to fix existing instances of this warning.