Closure warnings #4684

dfreedm · 2017-06-12T18:48:23Z

Full support for Closure Compiler

…t picked up as proto property (avoids initial binding value)

…ctor` (allows work to be done before `_initializeProperties` and is needed for proto/instance property initialization .

…e ignored

sorvell

Generally seems ok. We're inconsistent about when we're putting stuff in polymer-externs.js v. inline which is not ideal. I think we also need to add at least some comments somewhere about why/when we're using @suppress.

kevinpschaaf · 2017-06-16T14:50:12Z

What exactly is the difference between the type of warnings that go away with @unrestricted vs @suppress(missingProperties)?

kevinpschaaf · 2017-06-16T14:39:05Z

lib/elements/array-selector.html

+     * @extends {superClass}
+     * @implements {Polymer_PropertyEffects}
+     */
+    let propertyEffectsBase = Polymer.PropertyEffects(superClass);


This actually seems like a bug, since the mixin implements static get properties()... it should extend Polymer.ElementMixin I think (and revert the <link rel="import" href="../../polymer-element.html">)

Seems reasonable to me. That seems like a bug in master as well then.

kevinpschaaf · 2017-06-16T14:58:45Z

lib/legacy/legacy-element-mixin.html

@@ -188,7 +210,7 @@
       * @return {string} Serialized value
       */
      serialize(value) {
-        return this._serializeValue(value);
+        return this._serializeValue(value) || '';


This only exists for legacy usage (isn't called directly anymore), but technically speaking, it used to return undefined for false boolean properties, null, or undefined. So probably best to just make the return type {string|undefined}.

kevinpschaaf · 2017-06-16T15:00:30Z

lib/legacy/legacy-element-mixin.html

-        if (window.ShadyDOM && this.shadowRoot) {
-          this.shadowRoot.forceRender();
+        if (window.ShadyDOM) {
+          window.ShadyDOM.flush();


Curious why this is showing as a change? Just wasn't rebased recently?

This must have been missed in a merge conflict. I'll pull the master branch version.

kevinpschaaf · 2017-06-16T15:06:54Z

lib/mixins/property-effects.html

@@ -86,7 +89,7 @@
   * Runs all effects of a given type for the given set of property changes
   * on an instance.
   *
-   * @param {Object} inst The instance with effects to run
+   * @param {PropertyEffectsInstance | Polymer_PropertyEffects} inst The instance with effects to run


What does this mean? Seems like one or the other, no? Why doesn't it need to be a union of those two things?

Actually, PropertyEffectsInstance already extends Polymer_PropertyEffects so I can probably remove the type union here.

In general though, I would have to make a new type to do a union like you would want.

Looks like I need a bunch of casts now because a lot of methods in PropertyEffects class call out to these functions and the type doesn't exist in that scope.

dfreedm · 2017-06-16T17:04:19Z

@unrestricted allows for adding instance properties outside of constructor().

@suppress {missingProperties} suppresses the warnings generated by accessing properties that the compiler does not know that the class has.
Good examples include anywhere a class is dynamically assigned a property, or a type interface doesn't include private properties.

ChadKillingsworth · 2017-06-16T19:14:32Z

Man I'm glad to see this PR.

@Suppress {missingProperties} suppresses the warnings generated by accessing properties that the compiler does not know that the class has.
Good examples include anywhere a class is dynamically assigned a property, or a type interface doesn't include private properties.

Be careful when you do this that you don't inadvertently lie to the compiler about types. The type based optimizations will create some really hard to debug issues with blatantly wrong type information. Missing type information is not a problem for this though.

dfreedm · 2017-06-16T20:10:38Z

@ChadKillingsworth good points. I've been trying to use suppressions sparingly, mostly around edges where the lazy instantiation of class properties, but this is a good time to maybe try to shape the classes with nulls for those properties ahead of time.

ChadKillingsworth · 2017-06-16T20:59:52Z

@azakus You can also just add a stub definition for the compiler:

constructor() {
  /** @type {number} */
  this.myPropThatIsNotInitialized;
}

… types

It may break stuff in closure to use this much, so try to shape classes for closure directly. Use polymer pass v2 to remove warnings for properties defined in `properties` block

kevinpschaaf · 2017-06-29T21:29:16Z

lib/mixins/property-accessors.html

@@ -119,6 +120,28 @@

      constructor() {
        super();
+        /** @type {boolean} */


Try moving out to polymer-closure-types.html. The rule will be mixin property interface needs to be defined there, since it can't (easily) be auto-generated like the methods can.

kevinpschaaf · 2017-06-29T21:38:11Z

lib/mixins/property-effects.html

     * @unrestricted
     */
    class PropertyEffects extends propertyEffectsBase {

+      constructor() {
+        super();
+        /** @type {boolean} */


Investigate moving these definitions out to the extern HTML file, and then go back to using Polymer_PropertyEffects as the inst type in the private methods

This is hung up by some sort of internal compiler error :/

google/closure-compiler#2556

kevinpschaaf · 2017-06-29T21:57:19Z

externs/polymer-externs.js

Consider:

use this['__computeEffects'] in code instead of enum to keep them from being renamed, so that these don't have to be externed

that allows these and __dataHost to be moved to polymer-closure-types.html

which then allows closure-types.js to not need to be externed, and instead lumped alongside polymer-closure-types.html

kevinpschaaf · 2017-06-29T21:57:48Z

externs/polymer-externs.js

@@ -61,53 +64,20 @@ Polymer.sanitizeDOMValue;
 */
 function JSCompiler_renameProperty(string, obj) {}

-/** @record */
-function PolymerTelemetry(){}


Leave this externed

kevinpschaaf · 2017-06-29T22:04:16Z

lib/mixins/property-effects.html

@@ -1614,7 +1671,7 @@
        if (this.__propagateEffects) {
          runEffects(this, this.__propagateEffects, changedProps, oldProps, hasPaths);
        }
-        let templateInfo = this.__templateInfo;
+        let templateInfo = /** @type {TemplateInfo} */(this.__templateInfo);


Can remove cast since it's typed in ctor right now (to be moved to types file)

kevinpschaaf · 2017-06-29T22:05:51Z

lib/mixins/property-effects.html

      */
      notifySplices(path, splices) {
-        let info = {};
+        let /** PathInfo */ info = {path: ''};


Can eliminate the type info here since it should be structural.

kevinpschaaf · 2017-06-29T22:10:10Z

lib/mixins/template-stamp.html

-       * @param {Object} templateInfo Template metadata for current template
-       * @param {Object} nodeInfo Node metadata for current template.
+       * @param {!TemplateInfo} templateInfo Template metadata for current template
+       * @param {!NodeInfo} nodeInfo Node metadata for current template.
       * @param {string} name Attribute name
       * @param {*} value Attribute value


value arg should always be string; can remove cast below

ChadKillingsworth · 2017-07-03T12:01:50Z

I worked with this branch a bit over the weekend. A few notes:

Moving the interfaces from externs to source works great - except for the static properties. Since the compiler doesn't properly model static inheritance yet, leaving the static properties as externs seems appropriate to me.
Any extern or interface function that takes no arguments and returns nothing (it's only used for side effects) needs an @return {undefined} annotation. Otherwise, the compiler assumes it's of type function(...?):? - which is pretty horrible.
I recommend switching from @record to @interface. I had no problems doing that myself. @record doesn't get near the renaming that @interface does and you aren't currently using structural type checking.

Here's an example of the changes I made: ChadKillingsworth@6f27764

dfreedm · 2017-07-10T23:51:41Z

@ChadKillingsworth thanks!

dfreedm · 2017-07-11T17:52:24Z

Looks like the difference between putting all interfaces into externs and splitting statics out into externs is around 2kb in binary size.
Worthwhile, but not 100% necessary in my mind.
I think this can merge to master with the last two modifications described in #4684 (comment)

dfreedm · 2017-07-12T20:56:42Z

google/closure-compiler#2338 has merged, so now we can go ahead and merge this branch!

ChadKillingsworth · 2017-07-13T19:25:00Z

Yay!

Steven Orvell and others added 30 commits April 26, 2017 14:07

fix some closure warnings.

80f5442

Fix some closure warnings.

37abc4e

automate generating closure externs

89b1230

Fix some closure warnings.

5185554

Fix some closure warnings.

f1a1498

Fix more closure warnings.

fa9823f

gesture fixes

346e2d5

slighly better typing for mixin funcion

b3dfd38

Fix more closure warnings.

2627e63

Fix more closure warnings.

0b22959

Fix more closure warnings.

b686cd7

Fix more closure warnings.

0c3e3c5

Fix more closure warnings

d0f7812

Fix more closure warnings

f04d631

Fix dedupingMixin

2c9ffac

set isAttached constructor (for closure) but set to undefined so no…

6a995a2

…t picked up as proto property (avoids initial binding value)

LegacyElementMixin to @unrestricted

c1eda7a

initialize properties in _initializeProperties rather than `constru…

f15e4ee

…ctor` (allows work to be done before `_initializeProperties` and is needed for proto/instance property initialization .

inline cachingMixin into deduplicatingMixin

c3da507

fix typing for Polymer.Element

0b15293

remove TemplateStamp’s implicit dependency on _initializeProperties

f821e46

remove all "global this" warnings

c0ddc60

fix lint error

e5de178

Merge branch 'master' into closure-warnings

4698253

ensure latest closure, stay on polymer-build 1.1 until warnings can b…

7abd703

…e ignored

Merge branch 'master' into closure-warnings

db38807

update externs for merge, update dependencies

e927bc9

[ci skip] generating externs should be explicit

a8a57bf

[ci skip] paramaterize entries for closure task

3a80ad8

reduce closure warnings in TemplateStamp

c34ef0b

dfreedm requested a review from sorvell June 12, 2017 18:48

googlebot added the cla: yes label Jun 12, 2017

dfreedm added 3 commits June 15, 2017 14:43

Merge branch 'master' into closure-warnings

f7e2bd9

update closure log

344ebb4

remove polymer-element dependency introduced by a merge conflict

3b7eedb

sorvell suggested changes Jun 15, 2017

View reviewed changes

kevinpschaaf reviewed Jun 16, 2017

View reviewed changes

dfreedm mentioned this pull request Jun 16, 2017

Update the PolymerPass to support Polymer 2 in ADVANCED mode. google/closure-compiler#2338

Closed

dfreedm added 4 commits June 16, 2017 15:12

remove PolymerPropertyEffects type, inline DataTrigger and DataEffect…

7612df6

… types

Merge branch 'master' into closure-warnings

9b7e184

Remove @Suppress {missingProperties}

2efccb9

It may break stuff in closure to use this much, so try to shape classes for closure directly. Use polymer pass v2 to remove warnings for properties defined in `properties` block

zero warnings left

3e14a1d

kevinpschaaf requested changes Jun 29, 2017

View reviewed changes

dfreedm added 2 commits June 29, 2017 16:47

address some concerns from kschaaf

4465381

[ci skip] clean up mixin fn and regen externs

72022f2

dfreedm added 2 commits July 11, 2017 10:54

Meaningful closure fixes from @ChadKillingsworth

8804307

[ci skip] remove one new closure warning for updating closure

0cb560a

sorvell approved these changes Jul 13, 2017

View reviewed changes

sorvell merged commit 0b23e74 into master Jul 13, 2017

sorvell deleted the closure-warnings branch July 13, 2017 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closure warnings #4684

Closure warnings #4684

dfreedm commented Jun 12, 2017

sorvell left a comment

kevinpschaaf commented Jun 16, 2017

kevinpschaaf Jun 16, 2017

dfreedm Jun 16, 2017

kevinpschaaf Jun 16, 2017

dfreedm Jun 16, 2017

kevinpschaaf Jun 16, 2017

dfreedm Jun 16, 2017

kevinpschaaf Jun 16, 2017

dfreedm Jun 16, 2017

dfreedm Jun 16, 2017 •

edited

Loading

dfreedm commented Jun 16, 2017

ChadKillingsworth commented Jun 16, 2017

dfreedm commented Jun 16, 2017

ChadKillingsworth commented Jun 16, 2017 •

edited

Loading

kevinpschaaf Jun 29, 2017

kevinpschaaf Jun 29, 2017

dfreedm Jun 29, 2017

dfreedm Jul 1, 2017

kevinpschaaf Jun 29, 2017

kevinpschaaf Jun 29, 2017

kevinpschaaf Jun 29, 2017

kevinpschaaf Jun 29, 2017

kevinpschaaf Jun 29, 2017

ChadKillingsworth commented Jul 3, 2017

dfreedm commented Jul 10, 2017

dfreedm commented Jul 11, 2017

dfreedm commented Jul 12, 2017

ChadKillingsworth commented Jul 13, 2017

Closure warnings #4684

Closure warnings #4684

Conversation

dfreedm commented Jun 12, 2017

sorvell left a comment

Choose a reason for hiding this comment

kevinpschaaf commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfreedm Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

dfreedm commented Jun 16, 2017

ChadKillingsworth commented Jun 16, 2017

dfreedm commented Jun 16, 2017

ChadKillingsworth commented Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChadKillingsworth commented Jul 3, 2017

dfreedm commented Jul 10, 2017

dfreedm commented Jul 11, 2017

dfreedm commented Jul 12, 2017

ChadKillingsworth commented Jul 13, 2017

dfreedm Jun 16, 2017 •

edited

Loading

ChadKillingsworth commented Jun 16, 2017 •

edited

Loading