Safety of query decorators

[steverep]

Is it ever unsafe to use a getter created with the @query decorator? In other words, can it ever return null simply because the renderRoot hasn't been fully created yet?

If the answer is no, then I think this should be clearly added to the documentation. If the answer is yes (which is what I assume), then why not just build that safety into the decorator? For example:

if (!hasUpdated) {
  await updateComplete;
}
return this.renderRoot?.querySelector(selector) ?? null;

I'm struggling to think of a use case where it would be desired to query the shadow DOM before at least the first update completes, but please educate me if there is one.

[p-ob]

I believe your use case is precisely why queryAsync exists. Your solution changes the return signature to be a Promise that must be awaited. But queryAsync does that already.

In our solutions, we almost always use queryAsync, except when we have some sort of callback / event-based trigger where we know the element to be in the DOM.

https://lit.dev/docs/components/shadow-dom/#@query-@queryall-and-@queryasync-decorators

[steverep]

I believe your use case is precisely why queryAsync exists.

@queryAsync does not address that use case at all - it will always wait for at least one update. I'm referring to only waiting for the first update, and why you would ever want to not do that.

Your solution changes the return signature to be a Promise that must be awaited. But queryAsync does that already.

I'm no expert on promises so excuse the code as TBD. Irrespective of whether it returns a promise or an element, they are 2 different conditionals:

1. @query currently waits for nothing and you get what you get when it is called. It requires the user to be smart about where and when it is called otherwise it's a recipe for non-deterministic code.
2. @queryAsync waits for any pending update, even if that update won't affect the query result. (i.e. there's no way to gain performance and cache the result)

Maybe I'm being too simplistic, but I see query use cases falling into only 2 categories:

1. For whatever reason, it's only required to query the initial DOM state (but never before that). The result should be cached for performance benefit.
2. The query result may change with state and should be performed after any pending update (i.e. exactly what @queryAsync does now).

Then the API ought to reflect that.

[p-ob]

@query currently waits for nothing and you get what you get when it is called. It requires the user to be smart about where and when it is called otherwise it's a recipe for non-deterministic code.

Because @query does anything beside return a Promise, it cannot wait for anything. It's synchronous, and can only behave as such. You have to safely access the property yourself, or pivot to an async solution.

Let's use this as our example:

class MyElement extends LitElement {
  @query("#first")
  first;

  connectedCallback() {
    super.connectedCallback();
    
    // while the shadowRoot exists, the first render hasn't happened, and there's no DOM elements that match the query
    console.log(this.firstElement);
    // equivalent to this
    console.log(this.shadowRoot.querySelector("#first");

    this.updateComplete.then(() => {
      console.log(this.firstElement);
      console.log(this.shadowRoot.querySelector("#first");
    });
  }

  render() {
    return html`<div id="first">Hello</div>`;
  }
}

connectedCallback is running syncronously, and is running before the first render (e.g. before the firstUpdated callback would be used). Because this is running sync, lit cannot do what you are suggesting. this.firstElement is undefined, until the first render. The updateComplete Promise is available for you, however.

Additionally, @queryAsync does wait for any pending update. But if there is no pending update... it just immediately resolves. DOM APIs are relatively quick. I don't know what micro-optimizations you're shooting for, but isn't this.shadowRoot.querySelector a relatively cheap operation? I assume the browsers are doing some heavy caching themselves on a lot of this. If @queryAsync should be enhanced with an option to cache the result, though, similar to @query, that might be a good issue to open with the repo.

If you would like to safely use the sync approach, lit's documentation calls out needing to await this.updateComplete if you're trying to access that variable any time prior to firstUpdated.

It's worth noting, @query doesn't cache by default. It will execute the querySelector each time. You can change this, however:

@query("firstElement", true)
first;

Apologies if you already knew this; I haven't ever needed to dive this deep in to the decorators, so was fun to learn for myself.

[steverep]

Because @query does anything beside return a Promise, it cannot wait for anything. It's synchronous, and can only behave as such. You have to safely access the property yourself, or pivot to an async solution.

So going back to the first part of my original question, the answer is: "yes, it can be unsafe".

while the shadowRoot exists, the first render hasn't happened, and there's no DOM elements that match the query
connectedCallback is running syncronously, and is running before the first render (e.g. before the firstUpdated callback would be used). Because this is running sync, lit cannot do what you are suggesting. this.firstElement is undefined, until the first render.

This gets to the second part of my original question: Why would anyone ever want to do that? Should the safety of at least waiting for the first update be built-in? Studying the existing API more, you could modify the decorator and keep it synchronous:

if (!this.hasUpdated) {
  this.performUpdate();
}
return this.renderRoot?.querySelector(selector) ?? null;

That should add a little safety to the API by guaranteeing at least the first render completes prior to the query.

The updateComplete Promise is available for you, however.

Yeah, and that's built-in to @queryAsync as a shorthand, but there's no equivalent for only awaiting the first update (and no such promise). You'd have to be more clever and use this.hasUpdated || (await this.updateComplete) (and I'm not even 100% sure that's the correct way to do it).

I don't know what micro-optimizations you're shooting for, but isn't this.shadowRoot.querySelector a relatively cheap operation? I assume the browsers are doing some heavy caching themselves on a lot of this. If @queryAsync should be enhanced with an option to cache the result, though, similar to @query, that might be a good issue to open with the repo.

I'm just going by what the doc says about caching the result. Those questions are above my pay grade 🤷‍♂️ .

My current use case is actually a fairly simple one in theory. I'm using a modal dialog API that searches for an attribute to set initial focus inside the dialog. Often though, the desired element to focus is also a lit element, so I need to give it a custom focus method to be called by the dialog. In some cases, that pattern may be chained through several nested lit elements. I just want to make sure that chain never breaks because focus is called before everything has rendered.

Apologies if you already knew this; I haven't ever needed to dive this deep in to the decorators, so was fun to learn for myself.

I did and have been through the docs many times, but no worries. Thanks for hashing this out with me.

[sorvell]

This behavior is part of Lit's asynchronous rendering model. Lit's reactive update cycle is asynchronous because it generally results in ergonomic code that is naturally efficient. For example, where foo and bar are reactive properties el.foo = 5; el.bar = 6; is simple to write and causes the element to efficiently update/render only once.

The tradeoff is that if you do have an API that cares about the rendered state of an element (as in your use case), you have to use await el.updateComplete.

Since the asynchronous model is core to the design and very important for efficiency, we have avoided adding APIs that automatically trigger synchronous rendering, preferring instead to recommend using updateComplete and allowing users to trigger rendering explicitly if desired (e.g. by calling this.performUpdate() as you discovered).

Getting back to @query, with the caching option off, it functions as querySelector so a null value is not necessarily wrong if you use it to find an element that's conditionally rendered. If you use the property within the update lifecycle (e.g. in first/Updated) you don't have to use updateComplete. If, however, you have an imperative API that depends on the rendered state of the element, you do need to use updateComplete.

async focusSomeElement() {
  await this.updateComplete;
  this.myQueriedElement.focus();
}

Since this was a bit of a rough edge in some cases like this, we added @queryAsync to make it just a bit simpler.

async focusSomeElement() {
  (await this.myAsyncQueriedElement).focus();
}

Hope that helps.

[steverep]

I think that addresses all parts of my question, although I still think the safety of ensuring at least the first update should be built in to the decorators. Returning null because the render has conditionals is one thing, but returning null because it never rendered is another.

Honestly, I think what threw me off the most leading to this discussion is the lack of uniformity in the lit API:

• You can cache some queries but not others
• There is an an asynchronous version of one query but not others
• There's hooks for firstUpdated and updated, but you can only await the latter with updateComplete, i.e. there's no equivalent promise firstUpdateComplete
• The boolean hasUpdated is named inconsistently given it only refers to the first update

Maybe I should turn some or all of that into an issue?

[GHNewbiee]

... the lack of uniformity ...
... is named inconsistently ...

Things in programming (IT, CS, etc) do not "work" like in engineering! Lack of uniformity, inconsistencies, LOGICAL bugs (presented/justified as project's decisions), etc are present and will always be. They will never change for some reasons such as "backward compatibility", "lack of QA", "wrong mentality (publish the project as soon as possible even if it has 1K bugs / call project's decision any LOGICAL bug", etc.

It is totally waste of time to start an issue! Try to survive with what you can get! But, you could write a "General" note/hint about what you have found to be "wrong" including solution/suggestion/best practice/etc like that one by @AliMD.

For some things, I found in the past, I wrote a hint labelled " - Hint for beginners".

[AliMD]

That becouse I never use @query/@queryAsync and simply fill the private property in firstUpdated.
Best practice for performance and debugging.