-
Notifications
You must be signed in to change notification settings - Fork 50.2k
Enable modern event system and delete dead code #19230
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
Conversation
|
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 d04cd4d:
|
| it('flushes pending interactive work before extracting event handler', () => { | ||
| container = document.createElement('div'); | ||
| const root = ReactDOM.unstable_createRoot(container); | ||
| const root = ReactDOM.createRoot(container); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change this one. Git just thinks I renamed it whereas I actually deleted old test.
Kind of confusing why it uses unprefixed name. I thought that doesn't work anymore.
Details of bundled changes.Comparing: e3f4eb7...d04cd4d react-dom
react-native-renderer
ReactDOM: size: -24.8%, gzip: -21.7% Size changes (experimental) |
Details of bundled changes.Comparing: e3f4eb7...d04cd4d react-dom
react-native-renderer
ReactDOM: size: -24.8%, gzip: -21.7% Size changes (stable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good apart from packages/react-native-renderer/src/legacy-events/SyntheticEvent.js , where I think you muddled up the conditions. Update: Ignore.
|
Hmm. I'm not sure I follow. The native renderer uses the legacy system, right? We don't yet remove pooling on RN either. |
trueadm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah , ignore me. I got confused by which file.
In #19228, I forked legacy event system folder for DOM and Native.
In this PR, I am removing the
enableModernEventSystemflag altogether:enableModernEventSystem = true.enableModernEventSystem = false.This enables us to remove a bunch of files and codepaths in DOM. There's more to do there but this is already fairly invasive so I will send the rest as follow-ups.
You should review without whitespace. https://github.com/facebook/react/pull/19230/files?diff=split&w=1