Make compatible with closure advanced compilation by sorvell · Pull Request #374 · lit/lit-element

sorvell · 2018-12-15T00:26:51Z

uses JSCompiler_renameProperty where literal names are needed instead of using @export
The private _classProperties is no longer created by default. Instead _ensureClassProperties was added and is called to make sure that the specific class has its own storage for class properties.
Adds a test that ensure a subclass cannot alter a superclass. The condition tested here would only fail after merging Ensure user accessor is never overridden #359.
optimization: ensures LitElement is marked as finalized so that it does not ever try to finalize.

@exported

Currently, compiling lit-element with the Closure compiler results in a clean build but a number of hard JS errors due to a few issues: - static methods being collapsed (i.e. removed entirely) - Object.hasOwnPropertyName() looking for static properties that have been rewritten by the compiler - static property initializers not being run when @exported The last issue is a compiler bug being tracked internally, but all three need to be worked around before this will compile and run cleanly. This includes all of the necessary fixes, including addressing the last issue by moving the Object.hasOwnPropertyName('_classProperties') check to _finalize(). This is necessary to ensure that _classProperties is defined before it's iterated in observedProperties on an element that has no properties. Another option for making _finalized and _classProperties public would be to move to checking this._finalized and this._classProperties and not needing to access them by name - I thought that was out of scope for this but would be happy to change to that style if reviewers prefer.

Fully support Closure compiler including ADVANCED_OPTIMIZATIONS

* uses `JSCompiler_renameProperty` where literal names are needed instead of using `@export` * The private `_classProperties` is no longer created by default. Instead `_ensureClassProperties` was added and is called to make sure that the specific class has its own storage for class properties. * Adds a test that ensure a subclass cannot alter a superclass. The condition tested here would only fail after merging #359. * optimization: ensures `LitElement` is marked as `finalized` so that it does not ever try to finalize.

ghost · 2019-01-08T20:19:50Z

Does this need docs changes?

kevinpschaaf · 2019-01-10T00:04:21Z

@katejeffreys No, it's just making the type annotations and constructions compatible with closure; no real user-facing considerations there.

justinfagnani · 2019-01-09T23:54:43Z

src/lib/updating-element.ts


+/**
+ * When using Closure Compiler, JSCompiler_renameProperty(property, object) is
+ * replaced by the munged name for object[property] We cannot alias this


Suggested change

* replaced by the munged name for object[property] We cannot alias this

* replaced *at compile time* by the munged name for object[property] We cannot alias this

justinfagnani · 2019-01-09T23:58:16Z

src/lib/updating-element.ts

  /**
   * Maps attribute names to properties; for example `foobar` attribute
   * to `fooBar` property.
+   * @nocollapse


Can we make it clear in here somehow why some properties need nocollapse and others don't? I assume properties that we use only via own property lookup, and not static inheritance, don't need nocollapse. That distinction might be subtle for maintainers w/o everyday Closure experience. Maybe comment the exceptions?

No, not related to hasOwnProperty (that is dealt with by making sure we use JSCompiler_renameProperty anywhere we use a property by name. It's working around ES6 compiler bugs -- and definitely deserve a comment.

I believe this is right:

any static method needs @nocollapse to not be removed altogether

static fields need @nocollapse to get their initializers

@rictic Does the above sound correct?

That said, similar to why Steve removed the initializer for _classProperties, I'm not entirely sure why _attributeToPropertyMap and properties need initial values on UpdatingElement either... will take a look.

I think the simplest guidance is that all static methods and fields should have @nocollapse in order to preserve correct semantics.

jscompiler started the perspective that there is no inheritance of static members, and that static members can be rewritten to global variables. These assumptions don't hold up well with ES6+ code, and work is underway to handle them more correctly, but for the time being @nocollapse is the easiest way to tell it not to apply dangerous optimizations to them.

It's called @nocollapse because we're asking for these members not to be collapsed down into global variables.

Removed initial values for _attributeToPropertyMap and properties as well.

Added comment block describing when @nocollapse is needed.

justinfagnani · 2019-01-10T00:00:02Z

src/lib/updating-element.ts

-  static createProperty(name: PropertyKey,
-                        options:
-                            PropertyDeclaration = defaultPropertyDeclaration) {
+  /** @nocollapse */


I'm surprised we need nocollapse here. Is it because of this access? If so, seems like an unsafe optimization by Closure.

It's definitely unsafe

See above, all static methods need @nocollapse currently (closure bug workaround).

justinfagnani · 2019-01-10T00:01:33Z

src/lib/updating-element.ts

   * Memoized list of all class properties, including any superclass properties.
   */
-  private static _classProperties: PropertyDeclarationMap = new Map();
+  private static _classProperties?: PropertyDeclarationMap;


why remove the initializer?

UpdatingElement itself will never set anything into its _classProperties (since it never has properties); the set is created lazily during finalization on actual user subclasses

Can add a comment to that effect.

justinfagnani · 2019-01-10T00:04:04Z

src/lit-element.ts

+   * Ensure this class is marked as `finalized` as an optimization ensuring
+   * it will not needlessly try to `finalize`.
+   */
+  protected static finalized = true;


Shouldn't we initialize UpdatingElement.finalized to true too then?

It is already: https://github.com/Polymer/lit-element/blob/f1b9b61dfe577fc0a0cb1f1d8bc2f96a44d4c90b/src/lib/updating-element.ts#L218

rickcnagy and others added 4 commits November 13, 2018 21:03

Merge pull request #307 from rickcnagy/master

cded396

Fully support Closure compiler including ADVANCED_OPTIMIZATIONS

Format.

48ed5c9

sorvell requested review from dfreedm and kevinpschaaf December 15, 2018 00:26

googlebot added the cla: yes label Dec 15, 2018

sorvell added this to the 1.0.0 milestone Dec 15, 2018

kevinpschaaf added 2 commits December 17, 2018 16:07

Merge branch 'master' into closure-advanced

399384b

Merge branch 'master' into closure-advanced

ee0f4b1

Merge branch 'master' into closure-advanced

7fe8e78

sorvell requested a review from justinfagnani as a code owner January 8, 2019 22:58

Use JSCompiler_renameProperty for properties

f1b9b61

justinfagnani reviewed Jan 10, 2019

View reviewed changes

kevinpschaaf added 2 commits January 9, 2019 17:25

Minor updates based on review feedback.

a98377a

Remove @nocollapse from static fields with no initializer

a87e445

kevinpschaaf merged commit 231eb95 into master Jan 10, 2019

kevinpschaaf deleted the closure-advanced branch January 10, 2019 01:52

	* replaced by the munged name for object[property] We cannot alias this
	* replaced at compile time by the munged name for object[property] We cannot alias this

Comments

Conversation

sorvell commented Dec 15, 2018

Uh oh!

ghost commented Jan 8, 2019

Uh oh!

kevinpschaaf commented Jan 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinpschaaf Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kevinpschaaf Jan 10, 2019 •

edited

Loading