Skip to content

Conversation

@mdjastrzebski
Copy link
Member

@mdjastrzebski mdjastrzebski commented Jun 6, 2020

Summary

waitForElementToBeRemoved implementation with tests & typescript types.

  • implementation
  • tests
  • typescript types
  • docs

Resolves #375

Test plan

Added tests based on waitFor & veryfing getBy, getAllBy, queryBy & queryAllBy queries.

@mdjastrzebski mdjastrzebski requested a review from thymikee June 6, 2020 19:57
@mdjastrzebski mdjastrzebski marked this pull request as draft June 6, 2020 19:59
export default async function waitForElementToBeRemoved<T>(
expectation: () => T,
options?: WaitForOptions
): Promise<null> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if null is a proper type here. But we do not have anything meaningful to return.
RTL declares its version in TS as Promise<T> but uses return true in their impl.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO I would keep the Promise<T> return type and return the initial result of expectation call. Since in code we require that initially expectation is successful (returns something) and only then starts throwing/returning nothing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, let's do it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also logged issue in Dom TL that they have similar discrepancy.

@mdjastrzebski mdjastrzebski marked this pull request as ready for review June 8, 2020 07:35
@mdjastrzebski mdjastrzebski requested a review from cross19xx June 8, 2020 07:40
@thymikee thymikee force-pushed the feature/waitForElementToBeRemoved branch from e2fb263 to 1143370 Compare June 9, 2020 07:33
@thymikee
Copy link
Member

thymikee commented Jun 9, 2020

Used ErrorWithStack to get a nicer stack trace:

before after
Screen Shot 2020-06-09 at 09 30 14 Screen Shot 2020-06-09 at 09 30 08

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed is that using useFakeTimers('modern') doesn't work with neither of waitFor helpers. We need to figure this as well before it ships as default in Jest 27. But for now, this LGTM 👍

In order to properly use `waitFor` you need at least React >=16.9.0 (featuring async `act`) or React Native >=0.60 (which comes with React >=16.9.0).
:::

If you're using Jest's [Timer Mocks](https://jestjs.io/docs/en/timer-mocks#docsNav), remember not to use `async/await` syntax as it will stall your tests.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for later: we need to figure out why this happens.

@mdjastrzebski mdjastrzebski merged commit 300d16b into master Jun 10, 2020
@mdjastrzebski mdjastrzebski deleted the feature/waitForElementToBeRemoved branch June 10, 2020 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add waitForElementToBeRemoved API

3 participants