-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Add support for React.pure in ReactDOMServer #13855
Conversation
@@ -0,0 +1,112 @@ | |||
/** |
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.
There's a lot of setup cost for initializing these test modules. Can we put this together with some other tests since there's only a few of test cases here?
@@ -945,7 +946,8 @@ class ReactDOMServerRenderer { | |||
} | |||
if (typeof elementType === 'object' && elementType !== null) { | |||
switch (elementType.$$typeof) { | |||
case REACT_FORWARD_REF_TYPE: { | |||
case REACT_FORWARD_REF_TYPE: | |||
case REACT_PURE_TYPE: { | |||
const element: ReactElement = ((nextChild: any): ReactElement); | |||
const nextChildren = toArray( | |||
elementType.render(element.props, element.ref), |
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.
This is not correct. It is passing the element.ref
second argument. It shouldn't get the second argument in the pure
case.
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.
But #13822?
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 see that one. I'm not sure I approve. Let me think about it.
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 would be better to copy/paste the branch so that you can specialize it for the pure
use case. In this case, avoid passing the second argument.
Details of bundled changes.Comparing: 15b11d2...28f5c3b react-dom
scheduler
Generated by 🚫 dangerJS |
69ffbb8
to
bb472ec
Compare
Moved the tests and branched the REACT_PURE_TYPE logic. I'm still passing the forward ref stuff for now to keep the behavior in sync with ReactDOM (#13855). Let me know if you want to change this behavior. |
Yeah we're going to revert that and change so that:
@acdlite has details |
bb472ec
to
3cf899e
Compare
Updated my diff so |
3cf899e
to
345556e
Compare
I updated on top of #13903. This now uses a slow path to implement pure in a fully recursive form. This is unfortunate but that's the easiest given the structure of the partial renderer. It doesn't make it easy to make recursive custom types. But we're rewriting this whole engine soon anyway. |
This is very slow but meh. We're rewriting this whole thing anyway.
345556e
to
28f5c3b
Compare
* Add support for React.pure in ReactDOMServer * Unwrap pure wrappers by creating an additional element as a single child This is very slow but meh. We're rewriting this whole thing anyway.
* Add support for React.pure in ReactDOMServer * Unwrap pure wrappers by creating an additional element as a single child This is very slow but meh. We're rewriting this whole thing anyway.
Add support for REACT_PURE_TYPE in ReactDOMServer + tests