Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Pass container from findBy through to waitFor #868

Merged

Conversation

nathanforce
Copy link
Contributor

What:

Pass container from findBy into the waitFor call to support using findBy in non-global DOM environments.

Why:

The library should be agnostic of the runtime environment/globals when passing in a container.

How:

It looks like there are sensible defaults for container throughout the call stack, so I've just ferried the argument through.

Checklist:

  • Documentation added to the
    docs site n/a
  • Tests
  • Typescript definitions updated n/a
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 3, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 50c6353:

Sandbox Source
react-testing-library-examples Configuration

@@ -89,6 +90,12 @@ function waitFor(
await new Promise(r => setImmediate(r))
}
} else {
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was needed to keep these warnings intact. Without it, getWindowFromNode(container) throws and the error messages do not match the expected error. I thought it best to keep the warnings/behavior the same rather than update the test snapshots to match the new error message.

https://github.com/testing-library/dom-testing-library/blob/master/src/__tests__/base-queries-warn-on-invalid-container.js#L95-L127

@nathanforce
Copy link
Contributor Author

Worth noting that I had to commit with --no-verify for this as it looks like #848 may have introduced a type error?

[typecheck] types/__tests__/type-tests.ts(30,22): error TS2345: Argument of type 'number' is not assignable to parameter of type 'Matcher'.

@marcosvega91
Copy link
Member

Hi @nathanforce could you please rebase this branch with master?

@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #868 (50c6353) into master (71b4f46) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #868   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          946       951    +5     
  Branches       291       291           
=========================================
+ Hits           946       951    +5     
Impacted Files Coverage Δ
src/query-helpers.js 100.00% <100.00%> (ø)
src/wait-for.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71b4f46...50c6353. Read the comment docs.

@eps1lon eps1lon changed the title [fix] Pass container from findBy through to waitFor fix: Pass container from findBy through to waitFor Jan 12, 2021
@eps1lon eps1lon merged commit b4f1d45 into testing-library:master Jan 12, 2021
@eps1lon
Copy link
Member

eps1lon commented Jan 12, 2021

@all-contributors add @nathanforce for code

@eps1lon
Copy link
Member

eps1lon commented Jan 12, 2021

@nathanforce Thanks!

@allcontributors
Copy link
Contributor

@eps1lon

I've put up a pull request to add @nathanforce! 🎉

@github-actions
Copy link

🎉 This PR is included in version 7.29.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants