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

support document containers in cleanup #1330

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

quisido
Copy link

@quisido quisido commented May 20, 2024

What:

When container is set to document, the cleanup step fails:

TypeError: Cannot read properties of null (reading 'removeChild')

This is because the code assumes document.body exists:

    if (container.parentNode === document.body) {
      document.body.removeChild(container);
    }

In reality, both container.parentNode and document.body are null.

Why:

The tests erroneously fail.

How:

The attempt to remove the container from the body now only occurs when the body exists.

Checklist:

  • Documentation added to the
    docs site N/A
  • Tests
  • TypeScript definitions updated N/A
  • Ready to be merged

resolves #1329

Copy link

codesandbox-ci bot commented May 20, 2024

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 5fd2d54:

Sandbox Source
react-testing-library-examples Configuration

@eps1lon
Copy link
Member

eps1lon commented May 20, 2024

Please add a test.

I don't understand why document.body would be null? Isn't that always defined?

@quisido
Copy link
Author

quisido commented May 20, 2024

I don't know the setup, but is null when container is document. Locally, I fixed this by setting it to document.createElement('body').

I unfortunately do not have bandwidth to write a test for this, but the reproduction steps should be simple enough.

I ran into this specifically when writing a test for a NextJS root layout, which attempts to mount an HTML and body element. The docs say to use container: document when rendering your own HTML element, which fixed the previous error during test but introduced the OP error during cleanup.

@MatanBobi
Copy link
Member

@quisido thanks for taking the time to add this but we are not merging changes without proper coverage for them.

@quisido
Copy link
Author

quisido commented May 22, 2024

@quisido thanks for taking the time to add this but we are not merging changes without proper coverage for them.

Understandable. The PR is welcome to be updated. I opened an issue for it for tracking as well. I can try updating tests as time permits, but I wanted the problem and solution to both be documented.

@MatanBobi
Copy link
Member

I tried writing a test for this but it passes in our tests because document.body is not null. I'm trying to figure out what's the difference in the setup.

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