LitElement sub-optimal/broken with ES5 JSC compilation

[aomarks]

TL;DR LitElement compiled to ES5 is at best sub-optimal when the Closure JS Compiler property collapsing optimization is enabled, and (interestingly) actually broken when property collapsing is disabled. We might need to update the way we handle the finalized, properties, and styles static properties.

Collapsing enabled

LitElement has code roughly like this (actual code here):

class UpdatingElement {
  // UpdatingElement doesn't need to be finalized.
  static finalized = true;

  /** @nocollapse */
  static finalize() {
    if (this.hasOwnProperty('finalized')) {
      return;
    }
    this.finalized = true;
    // do class setup ...
    const superCtor = Object.getPrototypeOf(this);
    if (superCtor.finalize) {
      superCtor.finalize();
    }
  }
}

class MyElement extends UpdatingElement {}

Which JS Compiler will transpile roughly to:

var UpdatingElement = function() {};

UpdatingElement.finalize = function() {
  if (this.hasOwnProperty('finalized')) {
    return;
  }
  this.finalized = true;
  // do class setup ...
  const superCtor = Object.getPrototypeOf(this);
  if (superCtor.finalize) {
    superCtor.finalize();
  }
};

var MyElement = function() {
  UpdatingElement.apply(this, arguments);
};
MyElement.prototype = Object.create(UpdatingElement.prototype);
Object.setPrototypeOf(MyElement, UpdatingElement);
MyElement.finalize = UpdatingElement.finalize;

var UpdatingElement$finalized = true;
var MyElement$finalized = null;

So, the error here is that finalize() doesn't see that UpdatingElement has already been finalized, because the finalized static property was optimized to a variable -- but not cleverly enough to update the code in the finalize() static method. This is not super surprising, given
https://google.github.io/styleguide/jsguide.html#features-classes-static-methods implies that the compiler's understanding of static properties is pretty limited (and presumably this same limitation is why we already put @nocollapse on the finalize static method itself).

We get lucky that this doesn't seem to break anything right now, because all that happens is we unnecessarily finalize UpdatingElement (and LitElement, which is also pre-finalized). Presumably this is just a performance issue, not a correctness one, but you could imagine this same style of code causing a more serious breakage. There are two other static properties we do similar reflection on that I haven't looked into much yet: properties and styles.

Collapsing disabled

If we add a @nocollapse annotation to static finalized = true, or disable the property collapsing optimization all together (which is how I found this problem), then LitElement actually breaks, because those final two lines:

var UpdatingElement$finalized = true;
var MyElement$finalized = null;

change to:

UpdatingElement.finalized = true;
MyElement.finalize = UpdatingElement.finalize;

So, the static property is now (unnecessarily) copied onto the child constructor. MyElement will now think it's already finalized, which causes an exception (because its _attributeToPropertyMap never gets created). @rictic actually reported the issue with unnecessary property copying a while ago (google/closure-compiler#3177).

Proposal

We would need both JS Compiler issues to be fixed to allow LitElement to compile safely/optimally both with and without --collapse_properties enabled. We should see if we can advocate/help with that, but in the meantime, should we do something like this?:

class UpdatingElement {
  static finalize() {
    if (finalized.has(this)) {
      return;
    }
    finalized.add(this);
    // do class setup
  }
}

// Exported because LitElement needs to add itself to this set too.
export const finalized = new Set();
finalized.add(UpdatingElement);

This style also has the small advantage of not requiring a call to JSCompiler_renameProperty when doing the hasOwnProperty check (I omitted that earlier for simplicity).

[rictic]

Possibly related: google/closure-compiler#3177

[justinfagnani]

@aomarks Nice investigation. I think your suggested workaround sounds like the right approach for now.

[aomarks]

SG, I'll send a PR.

[sorvell]

Yeah, I think that makes sense too. Good work.