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: Null check added when resolving event's priority #31044

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

classicdocs
Copy link

@classicdocs classicdocs commented Sep 24, 2024

Summary

I wanted to migrate from React 17 to React 18 and experienced issues with rendering when using new root API.

When window.event is null, Uncaught TypeError: Cannot read properties of null (reading 'type')error can be thrown when calling render function.

✅ Example with React 17 where nothing breaks and component is rendered correctly:
https://jsfiddle.net/9zg3t4mc/1/

❌ Example with React 18 where error is thrown and component is not rendered:
https://jsfiddle.net/92wtjb0g/

How did you test this change?

I run yarn build and then use React-prod.js inside fiddle that is throwing an error and with this change render is working now.
image

Copy link

vercel bot commented Sep 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 27, 2024 4:08pm

@josephsavona
Copy link
Contributor

Hmm, the repro you provided is overriding a global to be null. I think that shouldn't ever happen in practice, though. Is there a legitimate case in some supported browser where the event can be null as opposed to undefined?

@classicdocs
Copy link
Author

classicdocs commented Sep 24, 2024

@josephsavona Thanks for quick answer. Yes, indeed it shouldn't be overwritten, but in real world I have experienced that client's app somehow is overriding this and I'm injecting a script into theirs app where I'm rendering react app and it will give me this error.

@josephsavona
Copy link
Contributor

Fair. This seems reasonable to add, but we also need a regression test. Can you add on here?

Copy link
Contributor

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

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

Requesting changes for a regression test

@classicdocs
Copy link
Author

@josephsavona I have added a test to cover this case. Cheers

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.

3 participants