Fix compatibility with JSC static property optimizations #735
Conversation
src/lib/updating-element.ts
Outdated
| /** | ||
| * Marks class as having finished creating properties. | ||
| */ | ||
| const finalized = new Set<typeof UpdatingElement>(); |
There was a problem hiding this comment.
Wouldn't a WeakSet be better here? As is, this'll strongly hold onto every LitElement.
There was a problem hiding this comment.
This would only hold on to class definitions (typeof UpdatingElement). But also, @aomarks is changing the PR to use a property again, just with the index operator and string literal to avoid Closure's bug. That'll allow the GC to collect classes if they every become freeable.
There was a problem hiding this comment.
Ah, I missed the static keyword in the code. 👍
src/lit-element.ts
Outdated
| @@ -85,7 +79,11 @@ export class LitElement extends UpdatingElement { | |||
|
|
|||
| /** @nocollapse */ | |||
| protected static finalize() { | |||
| super.finalize(); | |||
| if (this !== LitElement) { | |||
There was a problem hiding this comment.
Add a comment about this if statement.
Is it to match the previous finalized = true behavior?
There was a problem hiding this comment.
Yes, done. I didn't want to export the Set from updating-element (to prevent people from depending on that), and now that it's a property, I didn't want to have to know the special string property name outside of updating-element either.
There was a problem hiding this comment.
(Update: ended up wanting the special name in lit-element.ts after all, since it helps optimize the finalization stack, but I just copied the string instead of exporting something somebody might try and depend on)
src/lib/updating-element.ts
Outdated
| * Marks class as having finished creating properties. | ||
| * This property was previously used to record the classes that have already | ||
| * been finalized. This property is no longer used, but is still declared here | ||
| * for backwards compatability in TypeScript typings. |
There was a problem hiding this comment.
IMO breaking TypeScript typings isn't a breaking change for the library.
It would be better for someone to know that the finalized property no longer means anything. In any event, I doubt this will affect anyone, as the finalized property is pretty inside baseball.
|
PTAL @justinfagnani @rictic. We now use a string property instead of a Set, and (from discussion with @justinfagnani) I also moved the finalized check before recursing, instead of after, which optimizes out unnecessary super calls. |
Also moves the finalized check earlier, which optimizes out unnecessary super calls.
src/lib/updating-element.ts
Outdated
| /** | ||
| * The Closure JS Compiler doesn't currently have good support for static | ||
| * propertiy semantics where "this" is dynamic, so we use this hack to bypass | ||
| * any rewriting by the compiler. |
There was a problem hiding this comment.
There's no one bug for this right now.
src/lit-element.ts
Outdated
| @@ -85,7 +79,9 @@ export class LitElement extends UpdatingElement { | |||
|
|
|||
| /** @nocollapse */ | |||
| protected static finalize() { | |||
| super.finalize(); | |||
| // The Closure JS Compiler does not always preserve the correct "this" | |||
| // when calling static super methods, so explicitly bind here. | |||
src/lib/updating-element.ts
Outdated
| * propertiy semantics where "this" is dynamic, so we use this hack to bypass | ||
| * any rewriting by the compiler. | ||
| */ | ||
| const finalized = '__finalized__'; |
There was a problem hiding this comment.
Hm, if we're going this approach, I think we could do something like the following:
const finalized = 'finalized';
class UpdateElement {
protected static [finalized] = false;
protected static finalize() {
this[finalized] = true;
// ...
}
}
src/lit-element.ts
Outdated
| super.finalize(); | ||
| // The Closure JS Compiler does not always preserve the correct "this" | ||
| // when calling static super methods, so explicitly bind here. | ||
| super.finalize.apply(this); |
There was a problem hiding this comment.
Nit: call, since we don't need to pass parameters?
Fixes #732