Dom-testing-library: BREAKING: make waitForDomChange accept a callback

[kentcdodds]

Just an idea. When I use wait, I give it an assertion to wait for something to pass and I know my callback is called every 50ms until it passes. The nice thing about this is that we don't care what goes on during that time. We just wait for that assertion to pass.

However with waitForDomChange, we're just waiting for a single update to the DOM, but that's less helpful and arguably a bit more like implementation details because we shouldn't care how many DOM changes there are, we should wait until the DOM changes to a desirable state.

I'd like to think of waitForDomChange as a variant to wait which re-runs a callback whenever the DOM changes rather than arbitrarily every 50ms.

So it'd look like this (the same as waitForElement):

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

I expect that it would work similar to wait if you provide no callback. So this would be a breaking change only for people who are passing options (they would have to pass a callback of undefined ahead of the options, or even better, an actual callback making an actual useful assertion).

So this change would not break existing code that does this:

await waitForDomChange()

But it would break existing code that does this:

await waitForDomChange({container: el})

And that would need to change to:

await waitForDomChange(undefined, {container: el})

For the exact same behavior as before, or, preferably someone would give a callback with an assertion:

await waitForDomChange(() => expect(getByText(/status/i).toHaveTextContent('finished'), {container: el})

I think this is a good time to make this change considering we're about to release another breaking change: #373

[kentcdodds]

Oh, and I welcome your thoughts :)

[alexkrolick]

TBH I thought that wait, like waitForElement, used MutationObserver. Idea: make wait do that as well as the 50ms poll?

[kentcdodds]

Interesting idea. I wonder what the ramifications of that would be. I'd love to hear from others, but I'm liking this idea a lot. We could remove waitForDomChange entirely and just add mutation observer options to wait. I like it 👍

[eps1lon]

I rarely use wait* helpers so I don't have much of an opinion. I'm not entirely clear what problem this is solving though.

[le0nik]

I've used some wait* helpers and found waitForDomChange and waitForElementToBeRemoved which are based on MutationObserver to be unintuitive compared to other methods. I'll give an example.

Let's say clicking an element results in changes to the dom

fireEvent.click(element);
await waitForDomChange(container); // <--- this doesn't work, the change had already happened

const changePromise = waitForDomChange(container);
fireEvent.click(element);
await changePromise; // <--- this works, we started listening for changes before firing the event

Same problem with waitForElementToBeRemoved.

I like the approach of having a callback in waitForDomChange, which is immediately called initially and then keeps rerunning on dom changes. I realise that executing callback initially would either require passing an empty mutationsList or not passing it in all cases. Not sure how useful actual mutations are to users if they can query dom in the callback anyway.

waitForElementToBeRemoved could be deprecated then because waitForDomChange could do the same thing.

[alexkrolick]

@le0nik wait runs the query immediately, so waitForElement having that feature makes them very similar. I am still for putting all the functionality into just wait, treating the mutation observer as an optimization for when the callback resolves in less than the polling interval.

[afontcu]

Even though I'm not a heavy user of wait* queries, I do like the idea of reducing the exposed API by merging wait, waitForDomChange, and waitForElementToBeRemoved into a single method 👍

[kentcdodds]

:tada: This issue has been resolved in version 7.0.0-beta.2 :tada:

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[kentcdodds]

:tada: This issue has been resolved in version 7.0.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: