-
Notifications
You must be signed in to change notification settings - Fork 320
Adds @queryAsync decorator
#904
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
Conversation
Returns a promise this which resolves to the result of `querySelector` the given `selector` performed after the element's `updateComplete` promise is resolved.
src/lib/decorators.ts
Outdated
| const el = this.renderRoot.querySelector(selector); | ||
| // if this is a LitElement, then await updateComplete | ||
| if (el) { | ||
| await (el as LitElement).updateComplete; |
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 this good/required?
It sort of confuses the question "why should a user use asyncQuery?"
- Because you need a reference to a child element but it won't be there until after this element flushes?
- Because you've changed state on the child element and for some reason don't want to interact with it until after it's flushed?
The latter should be rare; flushing a child should normally be unobservable except for measurement (and we know that's fraught due to nested/chained updates). It's sort of lumping two use cases together, and given the second one is problematic, just want to discuss whether the child await is defensible vs. just pushing the entire measurement case to user responsibility.
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 also causes another microtask wait for every use, regardless of whether the element is a LitElement. And it doesn't work for other asynchronously rendering components like Stencil.
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.
Yup, removing this part.
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 is potentially very useful if made a bit more flexible:
export function asyncQuery(selector: string, waitFor?: (el) => Promise<unknown>) {
if (waitFor) { await waitFor(el); }
...Then one could do:
@asyncQuery('#foo', (e) => e.updateComplete) foo;Or the equivalent for Stencil...
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 a comment about supporting this in the future.
| } | ||
|
|
||
| /** | ||
| * A property decorator that converts a class property into a getter 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.
Why is this preferable over:
class {
@query('#foo') foo;
x() {
await this.updateComplete;
this.foo;
}
}ie, when would we tell someone to use one pattern over the other?
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.
Inside the element, this is arguably just slightly sugared. For a property that needs to be accessed external to the element, using @asyncQuery is much cleaner since the user doesn't have to know about updateComplete.
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.
Do we have use cases where an element wants to expose part of its internals like 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.
This is useful when a property that is a query can change with element state. It's preferable when the property is public so that the user does not have to know about the updateComplete promise.
src/lib/decorators.ts
Outdated
| const el = this.renderRoot.querySelector(selector); | ||
| // if this is a LitElement, then await updateComplete | ||
| if (el) { | ||
| await (el as LitElement).updateComplete; |
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 also causes another microtask wait for every use, regardless of whether the element is a LitElement. And it doesn't work for other asynchronously rendering components like Stencil.
| * | ||
| * class MyElement { | ||
| * @asyncQuery('#first') | ||
| * first; |
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 example should show the usage of this.first somewhere too.
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.
justinfagnani
left a comment
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
Only other thought is the name. I've gotten used to sync/async modifiers coming at the end of names. How's @queryAsync() sound?
src/lib/decorators.ts
Outdated
| const el = this.renderRoot.querySelector(selector); | ||
| // if this is a LitElement, then await updateComplete | ||
| if (el) { | ||
| await (el as LitElement).updateComplete; |
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 is potentially very useful if made a bit more flexible:
export function asyncQuery(selector: string, waitFor?: (el) => Promise<unknown>) {
if (waitFor) { await waitFor(el); }
...Then one could do:
@asyncQuery('#foo', (e) => e.updateComplete) foo;Or the equivalent for Stencil...
Fixes #903 by adding an
@asyncQuerydecorator.