-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
React 18 concurrent render functions / removal of findDOMNode / test updates #2330
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 bfab947:
|
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.
Thanks! This looks pretty good
* getTouchableNode: function() { | ||
* return this.touchableRef.current | ||
* } |
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.
Where is this used / implemented?
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.
Guidance from React is to remove findDOMNode
and use React.Ref instead. The Touchable
and ScrollResponder
mixins don't render themselves, so they become dependent on the concrete component to capture the root to a ref and provide it.
I didn't see any uses of Touchable
directly in this repository, so that seems to be only a user-facing breaking change. Nothing in this repository actually implements it.
ScrollResponderMixin
was used by ScrollView
, which already provided getScrollableNode
. It appears that it would only be user components using ScrollResponderMixin
that would have to be sure to implement getScrollableNode
now.
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 think after this is merged, it will also be a good time to remove those mixins as a legacy feature that will not carry forward with React 18 adoption.
The |
ReactDom does warn/error when that is called on a container used by createRoot. Someone using legacy mode is fine to call that method. We may have to provide some method of communicating to the root created by the new createRoot / hydrateRoot calls though? |
Removing The other two I can think of are:
|
@necolas I pushed a small change + test to return an object containing an unmount method from runApplication. It hides whether we used react 17 or 18 render/hydrate methods and can be used to expose access to the new root object returned by react 18 methods. |
bf36608
to
bfab947
Compare
@necolas Should I close this PR? I just saw the commit on 0.19-dev referencing this PR. |
Do you know on which update this will be fixed ? Thank you ! |
@JinbeiStudio This is part of 0.19 #2377 I don't know when that release is expected to go out. |
Looking to resolve the issues pointed out by #1529
The breaking changes come from the deprecation / removal of findNodeHandle, which doesn't appear to be a strictly necessary change to support React 18. Let me know if I should revert those changes.
Bump React dependency to 18.
Remove uses of
findNodeHandle
. Remove direct uses offindDOMNode
. MarkedfindNodeHandle
as deprecated for uses outside of this librarygetTouchableNode
- which returns the dom node of the touchable element.findNodeHandle
fallback, the dom node must be provided bygetScrollableNode
Support for concurrent mode in
AppRegistry
AppContainer
as the callback parameter to the new render/hydrate calls was removed.Using react testing library's render and act functions in tests