testing-library / testing-library/dom-testing-library

we may not need the initial check at the beginning of `waitForElementToBeRemoved`

Open
#1,265 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
3.3k
Forks
474
PR merge metrics
No merged PRs in 30d

Description

Describe the feature you'd like:

My suggestion is to remove the restriction that the element need to be present at the beginning of waitForElementToBeRemoved.

With the latest version of this library (v6.1.3), the target element of waitFoRElementToBeRemoved need to be present at the beginning.
Here is an example of my use case of waitForElementToBeRemoved. SomeComponent includes a message "some message" and a button to remove the mesage when it is clicked. (I'm sorry I implemented it with react, but it should work with other frameworks as well)

test('some executions after the message is removed works as expected', async () => {
  render(<SomeComponent />);
  const targetElement = getByText('some message');

  // an action to remove the message
  await user.click(getByRole('button', { name: 'remove message' }));

  // wait until the message is removed
  await waitForElementToBeRemoved(targetElement);

  // It is explicit that the following code is executed after the "some message" is removed
  doSomething();
})

The problem is waitForElementToBeRemoved causes an error on the initial check if the message is removed very soon after the button clicked.
Of course I could remove the sentence with waitForElementToBeRemoved, but I want to keep the sentence to make it clear that the following code is executed after the message is removed.

I understand that the reason you implemented the initial check is to make sure that the element IS REMOVED, not HAS NOT BEEN RENDERED (I saw this commend). But it may be overprotective.
In my example, it is obvious that the targetElement is rendered once because we get it with getByText('some message') query so we don't need to initial check.

I don't think we need to check the presence of the element (at least if the first argument is an element, even though I have no idea if it is a callback).

Suggested implementation:

Just removing the initialCheck for the element if the callback is not a function.

Describe alternatives you've considered:

We could use waitFor as like it is introduced in the document, but it is not a good match with expect.assertion.

Also, we could write a code as like await waitForElementToBeRemoved(queryByText('xxx')) and it passes even if it does not presence at the beginning with my suggestion. If we want to care this case, we may be able to create a eslint rule, as like "no-query-in-wait-for-element-to-be-removed"

Teachability, Documentation, Adoption, Migration Strategy:

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with src/wait-for-element-to-be-removed.js, especially the initialCheck linked in the issue. Review how the function handles an element versus a callback, then determine whether the initial presence restriction should be removed for element arguments. Done means the intended early-removal behavior is defined and covered by the project's relevant tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
testing-qa
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.