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

chore: Add document is null check in invariant #20887

Closed
wants to merge 6 commits into from

Conversation

zombieJ
Copy link
Contributor

@zombieJ zombieJ commented Feb 26, 2021

Summary

In jest, when a test is ended, it will make document as null instead of undefined. So the invariant of typeof document !== 'undefined' will miss the case of null.

Test Plan

Update ReactDOM-test.js for null case.

@sizebot
Copy link

sizebot commented Feb 26, 2021

Comparing: 2e8bbcb...42adb80

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 122.55 kB 122.56 kB = 39.48 kB 39.48 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 129.13 kB 129.14 kB = 41.53 kB 41.53 kB
facebook-www/ReactDOM-prod.classic.js = 406.12 kB 406.14 kB = 75.38 kB 75.39 kB
facebook-www/ReactDOM-prod.modern.js = 394.47 kB 394.49 kB = 73.49 kB 73.49 kB
facebook-www/ReactDOMForked-prod.classic.js = 406.13 kB 406.15 kB = 75.38 kB 75.39 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 42adb80

@zombieJ
Copy link
Contributor Author

zombieJ commented Feb 26, 2021

hi @acdlite, could you pls take a look? thanks~

@zombieJ
Copy link
Contributor Author

zombieJ commented Mar 1, 2021

cc @bvaughn

@zombieJ
Copy link
Contributor Author

zombieJ commented Mar 2, 2021

cc @gaearon

@zombieJ
Copy link
Contributor Author

zombieJ commented Mar 4, 2021

cc @rickhanlonii

@zombieJ
Copy link
Contributor Author

zombieJ commented Mar 8, 2021

cc @sebmarkbage

@zombieJ
Copy link
Contributor Author

zombieJ commented Mar 12, 2021

@bvaughn
Copy link
Contributor

bvaughn commented Mar 12, 2021

@zombieJ Don't spam mentions. There are a lot of PRs waiting for review. The team will review them when we get the opportunity.

@zombieJ
Copy link
Contributor Author

zombieJ commented Mar 14, 2021

@zombieJ Don't spam mentions. There are a lot of PRs waiting for review. The team will review them when we get the opportunity.

Thanks for reply.

@zombieJ
Copy link
Contributor Author

zombieJ commented Apr 23, 2021

Hi @bvaughn,
This PR has passed month no processed. Seems recently days many PR have been merged. What should I do next?

@rickhanlonii
Copy link
Member

Hey @zombieJ, thanks for the suggestion and I'm super sorry for the delay.

The real issue here is that this code is firing after jest has torn down the environment at all. To fix the core issue, we've created #20915, but that's a breaking change so we've been waiting until we're ready for the next release to land it.

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

Successfully merging this pull request may close these issues.

6 participants