Dom-testing-library: Make waitForElement always resolve to a truthy value.

[gnapse]

What?

I'd like waitForElement to always require a callback, and always resolve to a truthy value (an element).

Why?

As of today, this is the type definition for waitForElement:

export function waitForElement<T>(
  callback?: () => T,
  options?: {
    container?: HTMLElement
    timeout?: number
    mutationObserverOptions?: MutationObserverInit
  },
): Promise<T | undefined>

I question why the promise resolves to T | undefined.

The reason for the above type definition probably is that callback is optional. and when a callback function is not passed, the promise indeed resolves to undefined. But what's the point of calling waitForElement if we're never going to pass a callback?

Suggested implementation

Refactor the waitForElement implementation to require the callback or fail. And to not resolve to undefined in its absence. Update the type definitions to reflect this, and the promise resolve type to not allow undefined.

Describe alternatives you've considered:

In Javascript this is no big deal. Because you know, if you provided a callback, that by definition the promise will only resolves when this callback returns a truthy value (i.e. an element). But for TypeScript users, it can be cumbersome to have to explicitly handle the non-truths case that will actually never happen:

const table = await waitForElement(() => container.querySelector('table'));
if (!table) {
  // this will never happen
}
// ...

Admittedly, there's a more concise way, using table! from that point on. But again, why the need?

Teachability, Documentation, Adoption, Migration Strategy:

There's practically nothing to do here. Using waitForElement without a callback should not be a normal use case. So this would impact very few people.

Even the type definitions shown in the README for waitForElement do not match the promise resolve type in the actual type definitions. So in terms of documentation, this is already covered as it should've been in the code.

[kentcdodds]

I'm fine with that 👍

[gnapse]

Thanks! I'll submit a PR.

[gnapse]

Actually, is not that simple, but there's still room for improvement. Here's a note in the README about why the empty function is allowed:

The default callback is a no-op function (used like await waitForElement()). This can
be helpful if you only need to wait for the next DOM change (see mutationObserverOptions to learn which changes are detected).

What I'll do is that when no callback is provided, instead of defaulting to callback = undefined, I'll make it so T = undefined, so that you get a promise that resolves to undefined.

[gnapse]

Turns out the above revelation makes it simpler, in the sense that now the implementation need not be touched at all. Only the type definitions need to be updated.

However, my TypeScript knowledge may not be enough for this after the above revelation. I'm not sure how to express that T is undefined but only when the callback is not provided. Furthermore, it may be useful to express that T must otherwise be not-undefined.

I'm inclined to either do this:

export function waitForElement<T = undefined>(
  callback?: () => T,
  options?: {
    container?: HTMLElement
    timeout?: number
    mutationObserverOptions?: MutationObserverInit
  },
): Promise<T>

(which has the problem that providing a callback that returns undefined may trigger the same typings as not providing a function at all, when in fact that won't work as expected because the promise will never actually resolve).

Or define overloaded versions of the function:

export function waitForElement<T>(
  callback: () => T,
  options?: {
    container?: HTMLElement
    timeout?: number
    mutationObserverOptions?: MutationObserverInit
  },
): Promise<T>

export function waitForElement<undefined>(
  callback: undefined,
  options?: {
    container?: HTMLElement
    timeout?: number
    mutationObserverOptions?: MutationObserverInit
  },
): Promise<undefined>

I also find it weird that waitForElement can be used not to wait for an element, but to wait for the next mutation in the DOM, whatever it may be.

I think I'll leave this issue open instead, to continue the discussion.

[sompylasar]

@gnapse You can define function overloads, read more here: https://blog.mariusschulz.com/2016/08/18/function-overloads-in-typescript

[gnapse]

Yes, I know. I'm just not sure if the above reflects how the function really works.

[alexwendte]

@gnapse I agree with your last point about it being strange that waitForElement does not actually have to wait for an element. I love dom-testing-library and how declarative it is. This example seems a little bit less declarative than is normal. Perhaps there could be a separate waitForDomChange function or waitForElement could wait for a specific dom-change element (not sure what that would look like).

[sompylasar]

@themostcolm

@gnapse I agree with your last point about it being strange that waitForElement does not actually have to wait for an element. I love dom-testing-library and how declarative it is. This example seems a little bit less declarative than is normal. Perhaps there could be a separate waitForDomChange function or waitForElement could wait for a specific dom-change element (not sure what that would look like).

I like the idea of adding a separate function waitForDomChange, and removing the undefined-callback signature of waitForElement. What do you say @kentcdodds ?

[kentcdodds]

like the idea of adding a separate function waitForDomChange, and removing the undefined-callback signature of waitForElement.

I'm in favor 👍

[alexwendte]

I am almost finished with waitForDomChange. I am working on the documentation now. Would you rather it be called waitForDomUpdate?

But I guess waitForDomChange is more accurate. I just want to make sure 😄

[kentcdodds]

I vote for waitForDomChange 👍

[alexwendte]

Something interesting I found.

If you copy the test in waitForElement: 'does not get into infinite setTimeout loop after MutationObserver notification' and move it above the previous test block (or to simply not be the last one) it breaks all following tests saying, "Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout."

I thought I broke something on my own test when I was creating them, but I think it is inherent in that test.

[alexwendte]

Ah I fixed it. If you just throw a jest.useRealTimers() on to the end of that test, it fixes it :)

[gnapse]

Fixed in #118