-
Notifications
You must be signed in to change notification settings - Fork 319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refine API and refactor #132
Conversation
src/lit-element.ts
Outdated
|
||
/** | ||
* Override which sets up element rendering by calling* `_createRoot` | ||
* and `_firstRendered`. | ||
* Override which performs element rendering by calling the `render` method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Override which performs..." -> "Performs..." to follow the 3rd person, starts with a verb phrase style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gone
src/lit-element.ts
Outdated
* TemplateResult. By default this template is rendered into the element's | ||
* shadowRoot. This can be customized by implementing `_createRoot`. This | ||
* method must be implemented. | ||
* The implementation must return a `lit-html` TemplateResult. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
Invoked on each update to perform rendering tasks. This method must return a lit-html TemplateResult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lit-element.ts
Outdated
this.__isInvalid = true; | ||
super._invalidateProperties(); | ||
protected render(): TemplateResult { | ||
throw new Error('render() not implemented'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to mark this method (and class) as abstract
so IDEs will tell you to implement it. TS doesn't support default implementations on abstract methods though, so the error would have to go. Luckily, we'll still get an exception in update
, which I think is good enough. If we wanted to add nicer messages, we could check for the existence of render
in update
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/lit-element.ts
Outdated
private __isInvalid: Boolean = false; | ||
private __isChanging: Boolean = false; | ||
private _root?: Element|DocumentFragment; | ||
export class LitElement extends UpdatingElement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark as abstract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/lib/updating-element.ts
Outdated
* Creates a property accessor on the given prototype if one does not exist. | ||
* Uses `getProperty` and `setProperty` to manage the property's value. | ||
*/ | ||
function makeProperty(name: string, proto: Object) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer all functions that don't need a bindable this
to be const arrow functions. They should be very marginally faster in VMs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lib/updating-element.ts
Outdated
* return `this`. | ||
* @returns {Element|DocumentFragment} Returns a node into which to render. | ||
*/ | ||
protected createRoot() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createRoot
-> createRenderRoot
?
src/lib/updating-element.ts
Outdated
* @returns {Element|DocumentFragment} Returns a node into which to render. | ||
*/ | ||
protected createRoot() { | ||
this.root = this.attachShadow({mode : 'open'}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the contract is that this method should always set this.root
, why not just have this method return something and do the assignment in initialize
?
src/lib/updating-element.ts
Outdated
for (const p in (this.constructor as typeof UpdatingElement)._classProperties) { | ||
if (this.hasOwnProperty(p)) { | ||
// tslint:disable-next-line no-any | ||
const me = (this as any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do the cast on p
, since this line won't be won't be compiled out.
src/lib/updating-element.ts
Outdated
this.createRoot(); | ||
// save instance properties. | ||
for (const p in (this.constructor as typeof UpdatingElement)._classProperties) { | ||
if (this.hasOwnProperty(p)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment on why this if block is here
src/lib/updating-element.ts
Outdated
/** | ||
* Object with keys for all properties with their current values. | ||
*/ | ||
props: AnyObject = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be typed better as just props: this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't dug deeply, but it appears the coupling between lit-html
and the rest of the implementation here is very loose. If there's literally nothing lit-specific in the rest of the implementation, I'd argue in favor of publishing it as a separate library so that it could potentially be paired with a renderer other than lit. Could still live in the same repo, of course.
src/lib/render-helpers.ts
Outdated
* http://polymer.github.io/PATENTS.txt | ||
*/ | ||
|
||
import {camelToDashCase} from '@polymer/polymer/lib/utils/case-map.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the plan to copy/paste or reimplement here and remove this dependency on Polymer (along with the declared dependency in package.json
)?
Hey @sorvell, @rictic mentioned this PR in a chat about other lit-element things. Looking through this, I’m noticing that it seems like there’s no equivalent for |
* Property options have been updated to now include: `attribute`: set to explicitly false to not observe, to a string to customize name, or true or absent to observe lowercased name; `type`: if a function, used to deserialize attribute value, otherwise can be an object with {fromAttribute: deserializing function, toAttribute: serializing function}; `reflect`: if true, property reflects to attribute using type.toAttribute if present; `shouldInvalidate`: function used to determine if a change should trigger invalidation. * setting properties in update now does not trigger invalidation and does not issue a warning. This can and should be done to compute values before updating. * finishUpdate has been added to perform post update tasks like direct dom manipulation. It is an async function which will be awaited before resolving the updateComplete promise. Setting properties in finishUpdate will trigger invalidation and this will finish before the updateComplete promise is resolved. * no longer depends on polymer.
@justinfagnani PTAL Lots of changes per offline discussion:
TODO
|
@rickcnagy the pattern here would be to override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a full review, but a couple of quick comments...
src/lib/updating-element.ts
Outdated
* implemented to render and keep updated DOM in the element's root. | ||
* * @param _changedProperties changed properties with old values | ||
*/ | ||
protected update(_changedProperties: AnyObject) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this abstract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/lib/updating-element.ts
Outdated
this._validationState = ValidationState.Valid; | ||
// During finishUpdate, setting properties does trigger invalidation, | ||
// and users may choose to await other state, like children being updated. | ||
await this.finishUpdate(changedProps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this as:
if (this.finishUpdate !== undefined) {
await this.finishUpdate(changedProps);
}
will mean one less microtask in the case where there's no finishUpdate
defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/lib/updating-element.ts
Outdated
* `invalidate` and `updateComplete` Proimses. | ||
* * @param _changedProperties changed properties with old values | ||
*/ | ||
protected async finishUpdate(_changedProperties: AnyObject) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this abstract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did, but TypeScript doesn't seem to support an optional abstract method. Left it in so we can verify that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks like it'd just be optional then, with no implementation. ie, optional => abstract.
src/lib/updating-element.ts
Outdated
// did not trigger more work). | ||
if (this._validationState === ValidationState.Valid) { | ||
while (this._validateResolvers.length) { | ||
const resolver = this._validateResolvers.pop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the variable:
this._validateResolvers.pop()!();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gone.
src/lib/updating-element.ts
Outdated
} | ||
this._validationState = ValidationState.Invalid; | ||
// Make a new validate promise and put the resolver on the stack. | ||
this._validatePromise = new Promise((resolve) => this._validateResolvers.push(resolve)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a stack of resolvers? Could use some docs around it's relation to reentrant invalidate
calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gone.
src/lib/updating-element.ts
Outdated
// Only resolve the promise if we finish in a valid state (finishUpdate | ||
// did not trigger more work). | ||
if (this._validationState === ValidationState.Valid) { | ||
while (this._validateResolvers.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you resolve all the Promises at once, is seems like we could just share a single Promise. Are they ever resolved separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that had occurred to me as well and I forgot to clean it up. Will do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* mark update/finishUpdate as abstract * remove stack of promises in `invalidate`
README.md
Outdated
and renders declaratively using `lit-html`. | ||
|
||
* **Setup properties:** LitElement supports a static `properties` getter in which element property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest leading with a more generic description of properties:
LitElement supports observable properties which may trigger an update when set. These properties can be written in a few ways:
- As class fields with the
@property
decorator (with links to class fields and decorators proposals?), if you're using a compiler that supports them, like TypeScript or Babel.- With a static
properties
getter or property.- By manually writing getters and setters
And then go into each method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
* **Setup properties:** LitElement supports a static `properties` getter in which element property | ||
accessors are described. For each property, an object configures settings where the options are: | ||
|
||
* attribute: if false, do not add to observedAttributes, if true or absent, observe the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add backticks around the property names: attribute
, observedAttributes
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the leading word after the colon should be capitalized, and each "If...` clause a sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing the camel casing automatically, why not also allow attribute: 'my-custom-name'
as well, so that I can choose the mapping name that I want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be exactly what you are doing in the TS example:
@property({attribute: 'more-info', type: (value: string) => `[${value}]`})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, you seems to be doing exactly what I wanted, but the documentation doesn't mention that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
@@ -70,8 +81,8 @@ and renders declaratively using `lit-html`. | |||
1. Create a class that extends `LitElement`. | |||
1. Implement a static `properties` getter that returns the element's properties | |||
(which automatically become observed attributes). | |||
1. Then implement a `_render(props)` method and use the element's | |||
current properties (props) to return a `lit-html` template result to render | |||
1. Then implement a `render()` method and use the element's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a minimal TypeScript example should be right up top to sell things a little.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
* `finishUpdate(changedProps)` (protected): Called after element DOM has been updated and | ||
before the `updateComplete` promise is resolved. The `changedProps` argument is an object | ||
with keys for the changed properties pointing to their previous values. This is an | ||
async function which is *awaited* before resolving the `updateComplete` promise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Async or not is an implementation detail of a function. I'd just say it can return a Promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lib/render-helpers.ts
Outdated
classInfo: {[name: string]: string|boolean|number}) { | ||
const o = []; | ||
for (const name in classInfo) { | ||
const v = classInfo[name]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move classInfo[name]
into the if condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lib/updating-element.ts
Outdated
* Change function that returns true if `value` is different from `oldValue`. | ||
* This method is used as the default for a property's `shouldChange` function. | ||
*/ | ||
// tslint:disable-next-line no-any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using unknown
should remove the need for the lint directive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lib/updating-element.ts
Outdated
return old !== value && (old === old || value === value); | ||
}; | ||
|
||
enum ValidationState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really prefer avoiding enums and namespaces in TypeScript. They're the only non-type extensions to JS, and I think generally considered a mistake. Unless you need the bidirectional mapping of enums, using a plain object or a union type of literals usually suffices, ie:
type ValidationState = 'disabled' | 'valid' | 'invalid';
or:
const disabled = 0;
const valid = 1;
const invalid = 2;
type ValidationState = typeof disabled | typeof valid | typeof invalid;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lib/updating-element.ts
Outdated
* to `fooBar` property. | ||
*/ | ||
private static _attributeToPropertyMap: AttributeMap = {}; | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit, here and throughout: I think it's easier to read with a blank line before doc comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lib/updating-element.ts
Outdated
/** | ||
* Memoized list of all class properties, including any superclass properties. | ||
*/ | ||
static _classProperties: Properties = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this and properties
? Can't the decorator just write to properties
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a memoization for efficiency so we don't need to look at the superclass properties at runtime. This is info is really frequently needed: when a property invalidates or deserializes, for example.
src/lib/updating-element.ts
Outdated
} | ||
// finalize any superclasses | ||
const superCtor = Object.getPrototypeOf(this); | ||
if (superCtor.prototype instanceof UpdatingElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than an instanceof check, can we rely on the presence of the _finalized
method? This would make subclassing work across multiple versions of this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
|
||
* When the element is first connected or a property is set (e.g. `element.foo = 5`) | ||
and the property's `shouldInvalidate(value, oldValue)` returns true. Then | ||
* `invalidate()` tries to update the element after waiting a microtask. Then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does regular users understand about microtasks? Like to explanation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
* When the element is first connected or a property is set (e.g. `element.foo = 5`) | ||
and the property's `shouldInvalidate(value, oldValue)` returns true. Then | ||
* `invalidate()` tries to update the element after waiting a microtask. Then | ||
* `shouldUpdate(changedProps)` is called and if this returns true which it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comma before which
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
demo/lit-element.html
Outdated
get zot() { return this.getProperty('zot'); } | ||
|
||
set zot(value) { | ||
this.setProperty('zot', Number(value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if the example explained exactly what you are trying to accomplish here. Like why is this useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably something to cover in a getting started guide, but tweaked it a bit.
src/lib/updating-element.ts
Outdated
type?: AttributeProcessor|Function; | ||
/** | ||
* Describes if setting the property should be reflect to the attribute value. | ||
* If true, then if present the `type.toAttribute` function is used to serialize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this English a bit hard to follow. "then if present the ... is used" - at least add a comma after "present".
An example would probably also make this more clearer, like why is this useful and when to use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
src/lib/updating-element.ts
Outdated
* This function takes the new and oldValue and returns true if invalidation | ||
* should occur. If not present, a strict identity check is used. | ||
*/ | ||
shouldInvalidate?(value: unknown, oldValue: unknown): boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a motivating example on when you don't want something to invalidate would be quite useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lib/updating-element.ts
Outdated
private _changedProps: AnyObject|null = null; | ||
|
||
/** | ||
* Node or ShadowRoot into which element DOM should be renderd. Defaults |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rendered - missing e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
The current convention is private __isChanging: Boolean = false;
private _root?: Element|DocumentFragment;
_firstRendered() {}
protected _createRoot(): Element|DocumentFragment |
Also test subclass/superclass do not improperly interact.
Reflecting attributes in the property setter does not work since Safari does not allow an element to produce attributes in the constructor. We will need to move this to |
README.md
Outdated
* shouldInvalidate: optional function which should return true if setting the property | ||
should cause the element to invalidate causing it to asynchronously update. | ||
* **Setup properties:** LitElement supports observable properties which may trigger an | ||
update when set. These properties can be written in a few ways: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
README.md
Outdated
for calling methods on rendered elements, for example focusing an input: | ||
`this.shadowRoot.querySelector('input').focus()`. The `changedProperties` argument is a Map | ||
with keys for the changed properties pointing to their previous values. | ||
Note, calling `super.update()` is required. Before calling `super.upadate()`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling: upadate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
README.md
Outdated
does by default: | ||
* `update(changedProperties)` is called to update the element. | ||
Note, setting properties inside `update()` will set their values but | ||
will *not* trigger `invalidate()`. This calls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will after calling super.update()
right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
README.md
Outdated
will *not* trigger `invalidate()`. This calls | ||
* `render()` which should return a `lit-html` TemplateResult | ||
(e.g. <code>html\`Hello ${world}\`</code>) | ||
* `finishFirstUpdate()` is then called to do post *first* update/render tasks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finishFirstUpdate()
was also removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -13,7 +13,7 @@ | |||
"build": "tsc", | |||
"gen-docs": "typedoc --readme none --mode modules --excludeNotExported --excludePrivate --exclude **/*_test.ts --out ./docs/api .", | |||
"test": "npm run build && wct", | |||
"checksize": "uglifyjs lit-element.js -mc --toplevel | gzip -9 | wc -c", | |||
"checksize": "rollup -c ; rm lit-element.bundled.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you need a rollup config?
See https://github.com/Polymer/lit-html/blob/master/rollup.config.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
of whether or not any property changes are pending. | ||
## Advanced: Update Lifecycle | ||
|
||
* When the element is first connected or a property is set (e.g. `element.foo = 5`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the nesting here to make this visually more complicated than it is. Also the "thens", "Notes" and other connector words.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrote
src/lib/updating-element.ts
Outdated
} | ||
|
||
/** | ||
* Object that describes accessors to be created on the element prototype. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: You don't need phrases like "Object that", you can just start with "Describes...". Also, the second sentence doesn't describe the interface, but some other thing that uses it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrote
src/lib/updating-element.ts
Outdated
private static _finalized = true; | ||
|
||
/** | ||
* Memoized result of computed observedAttributes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this have to be memoized? For customElements.define
it's torn off at define
time and remembered by the browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idea was for calls to super.observedAttributes
in overrides but probably not needed. Can remove.
if (this.prototype.hasOwnProperty(name)) { | ||
return; | ||
} | ||
const key = typeof name === 'symbol' ? Symbol() : `__${name}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the key should be a Symbol either way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will perf test.
src/lib/updating-element.ts
Outdated
* Called when a property value is set and uses the `shouldInvalidate` | ||
* option for the property if present or a strict identity check. | ||
*/ | ||
private static _propertyShouldInvalidate(value: unknown, old: unknown, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we don't have the property name here, I'd consider a name like _valueShouldInvalidate
, as _propertyShouldInvalidate
sounds like the function you'd get for a specific property (the third arg here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/lib/updating-element.ts
Outdated
|
||
/** | ||
* Updates the element. By default this method reflects property values to attributes. | ||
* It should be implemented to render and keep updated DOM in the element's root. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"implemented" -> "overridden"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lit-element.ts
Outdated
* @param props Current element properties | ||
* @param changedProps Changing element properties | ||
* @param prevProps Previous element properties | ||
* Override which performs element rendering by calling the `render` method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These will be the docs shown in the doc viewer. I don't think "Override which..." provides a lot of information. I'd copy a lot of the docs from UpdatingElement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lit-element.ts
Outdated
this.__resolveRenderComplete(true); | ||
protected update(_props: PropertyValues) { | ||
super.update(_props); | ||
if (typeof this.render === 'function') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A method can't both be abstract and have an implementation. For TypeScript users, they'll get an error in their IDE that they should implement, which is better because it's sooner.
this.__isInvalid = true; | ||
super._invalidateProperties(); | ||
} | ||
protected firstRendered?(): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
…ng. Fixes tests on Edge / IE.
This helps typescript users get a nice warning if they forget to implment `render`.
src/lib/updating-element.ts
Outdated
// Note: special case `Boolean` so users can use it as a `type`. | ||
const toAttribute = options.type === Boolean ? toBooleanAttribute : | ||
(options.type && (options.type as AttributeSerializer).toAttribute || String); | ||
return (typeof toAttribute === 'function') ? toAttribute(value) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaults to String, so check is unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/lib/updating-element.ts
Outdated
/** | ||
* Validates the element by updating it via `update`. | ||
*/ | ||
private _validate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"validate" to me means "check if valid or not", not "make valid"
Consider "flush" to mean "flush all queued invalidated (dirty) properties by calling update"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}; | ||
|
||
const microtaskPromise = new Promise((resolve) => resolve(true)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's comment what these states are relevant for.
Also suggest changing STATE_IS_UPDATING -> STATE_IS_INVALID (or STATE_IS_DIRTY)
"Is updating" doesn't seem like good nuance for "has an invalidated, dirty property"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fairly arcane, but looks like immediately invoking async function assigned to a getter that uses `super` loses `this` context on Safari 10 so working around by using Promises directly instead.
Add workaround for Promise timing bugs. Promise microtasks are not guaranteed to go before tasks.
WIP (do not merge). Tests do not pass.
initialize
=>createRoot
connectedCallback
=>enableUpdate
=>invalidate
invalidate
=>shouldUpdate
=>update
updateComplete
promise