Should understand '@override' for external APIs.

[blickly]

Upon compiling a Maps V3 API based code, I ran into an issue, where CC 
would rename a method ("draw") overriden from the Maps V3 API.
Obviously, I need the method name to remain intact for the override to work 
properly and the child class to be functional.
IMHO, methods annotated with "@override" and overriding a method coming 
from an external source - that thus cannot be changed by the closure 
compiler - should not be renamed.

---[extern.js, excerpt from Maps V3 API]---
/** @constructor */
google.maps.MVCObject = function() { };
/**
* @constructor
* @extends {google.maps.MVCObject}
*/
google.maps.OverlayView = function() {};
google.maps.OverlayView.prototype.draw = function() {};
---[test.js]--
/**
* @extends google.maps.OverlayView
*/
TestClass = function() { };
TestClass.prototype = new google.maps.OverlayView();
/**
* @override
*/
TestClass.prototype.draw = function(){window.alert("Added to map.");}

window["TestClass"] = TestClass;
--[result.js]--
TestClass = function() {
};
TestClass.prototype = new google.maps.OverlayView;
window.TestClass = TestClass;
--[]--

The test case I setup is even worse: it completely removed the overridden 
function, since it was not referenced. I propose that methods with an 
@override annotation should always be considered exported if they override 
a method coming from an external source.

Original issue reported on code.google.com by erich.schubert on 2010-01-31 20:58:08

[blickly]

I concur - I've also dealt with this.

As a workaround you can use the following notation:
/**
* @override
* @this {TestClass}
*/
TestClass.prototype['draw'] = function(){window.alert("Added to map.");}

Original issue reported on code.google.com by chadkillingsworth@missouristate.edu on 2010-02-01 01:39:36

[blickly]

I tried that, too. But it seems than when running the javascript un-compiled, this 
workaround isn't doing it all the way. Instead I sometimes had to also replace all

calls to the method by class["method"](args)
But I didn't investigate that further, I'm currently using

MyClass.prototype.method = function(){};
MyClass.prototype["method"] = MyClass.prototype.method;

That is a bit annoying, but it works. It does lead to redundant code after 
compilation though:

c.prototype.draw=function(){this.idle()};
c.prototype.draw=c.prototype.draw;

The second statement obviously could be left out by the compiler ...

Original issue reported on code.google.com by erich.schubert on 2010-02-01 01:48:17

[blickly]

If you are directly calling the method you'd need to do:

/** @override */
TestClass.prototype.draw = function(){window.alert("Added to map.");}
TestClass.prototype['draw'] = TestClass.prototype.draw;

It will work both in compiled and uncompiled versions that way.

Original issue reported on code.google.com by chadkillingsworth@missouristate.edu on 2010-02-01 02:30:32

[blickly]

Hmm looks like I misread your post. That's what you ended up.

Yes it's redundant, but not by much. You would only need that second notation for
methods you call directly. If you are extending google.maps.OverlayView, you probably
won't be direction calling onAdd or onRemove directly so they can use the first notation.

Original issue reported on code.google.com by chadkillingsworth@missouristate.edu on 2010-02-01 02:34:03

[blickly]

I'm going to try to summarize this as 3 feature requests:

If you @override on a subclass of an extern means the compiler should assume the
method exists on the superclass, and 
(a) not rename it, and
(b) not remove it.

Right now, you have to add some boilerplate to keep the code in
c.prototype.draw=c.prototype.draw;
and (c) that code should be removed.

Is that accurate?

(a) will not happen because we already have a way to declare the external method so
that it is not renamed. If you compile with --warning_level=VERBOSE, you will get a
warning that you @overrided a method that does not exist. 

(b) will not happen because many people write libraries that override the external
Maps API. They would be very upset with us if these methods did not get removed by
ADVANCED mode, since that is largely the point of ADVANCED mode.

(c) is a good point.

Original issue reported on code.google.com by Nicholas.J.Santos on 2010-02-01 04:06:51

[blickly]

(a) The method DOES exist (at least in the external file, could be abstract):
google.maps.OverlayView.prototype.draw = function() {};

The whole point is that by renaming the method, the closure compile basically breaks

the override.

(b) I see your point here, as in giving the user a way of declaring which methods of

an object to keep, even when they are overrides. But I'm not entirely convinced, 
since that will probably just cause people to export any method they override, 
because the closure compiler is unabel to detect these kinds of dependencies, so they

need to be given manually.

Maybe it would be worth to add an optimization flag for this, or differentiate 
between "advanced" (removing dead code and renaming methods, but with implicit 
exports such as @override-of-external) and "aggressive" mode (only uses explict 
exports)

Original issue reported on code.google.com by erich.schubert on 2010-02-01 08:53:26

[blickly]

Maybe it would also make sense to specify this in the externals file somehow.
Such as annotating OverlayView.setMap with "@calls draw", so the closure compiler 
could infer from setMap() being invoked that draw() will also be invoked (and with

this name, since it's in external ...)

Original issue reported on code.google.com by erich.schubert on 2010-02-01 09:43:07

[blickly]

a) The compiler functions as expected here. I expect/want a warning when I override
a
method that does not exist.

b) Many event handlers in the Maps API must exist for an extended object.
Specifically, the OverlayView object is regularly extended. You must overide the
methods onAdd and onRemove that are called by the maps API - not by your code. These
should neither be renamed or removed.

I've gotten around the issue by using the notation:
/**
* @override
* @this {TestClass}
*/
TestClass.prototype['onAdd'] = function() {};
The @this tag is required, otherwise the compiler gives a warning about dangerous use
of the global this object as it does not recognize the quoted notation as extending
the prototype. I also think that the @inheritDoc tag is not recognized.

What about not renaming methods that have the @inheritDoc tag? Could it be correctly
inferred that this symbol is both required and that it should not be renamed when
@inheritDoc is present?

Original issue reported on code.google.com by chadkillingsworth@missouristate.edu on 2010-02-01 13:41:50

[blickly]

I have to disagree that the compiler works as expected. (wrt. a)

I had named the function "draw", and annotated it with "@override".
After renaming, it was called e.g. "a", but the super class does not have a method

"a" (but only "draw"). So after the advanced optimization, the @override was invalid,

but I did NOT get a warning.
As long as the overridden method is renamed the same way that obviously is okay (and

should not cause a warning).

I'm pretty sure i tried with VERBOSE, and did not get a warning. I'll try again 
tonight or tomorrow night.

Original issue reported on code.google.com by erich.schubert on 2010-02-01 13:49:28

[blickly]

For a), I was stating that if you use @override on a method that does not exist on
the superclass, the compiler should and does give a warning. I can verify that this
behavior occurs. This is the working as expected part.

Original issue reported on code.google.com by chadkillingsworth@missouristate.edu on 2010-02-01 14:29:21

[blickly]

Issue tracking has been migrated to github. Please make any further comments on this
issue through https://github.com/google/closure-compiler/issues

Original issue reported on code.google.com by blickly@google.com on 2014-05-01 18:31:49

• Labels added: Restrict-AddIssueComment-Nobody

[blickly]

(No text was entered with this change)

Original issue reported on code.google.com by blickly@google.com on 2014-05-01 18:34:11