Call and Return components should use ReactElement #11834
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ReactChildFiber contains lots of branches that do the same thing for different child types. We can unify them by having more child types be ReactElements. This requires that the `type` and `key` fields are sufficient to determine the identity of the child. The main benefit is decreased file size, especially as we add more component types, like context providers and consumers. This updates Call and Return components to use ReactElement. Portals are left alone for now because their identity includes the host instance.
acdlite
force-pushed
the
use-elements-for-call-and-return
branch
from
d0bae7a
to
7aa0d19
Compare
gaearon
reviewed
Dec 12, 2017
| @@ -621,11 +621,6 @@ class ReactDOMServerRenderer { | |||
| 'Portals are not currently supported by the server renderer. ' + | |||
| 'Render them conditionally so that they only appear on the client render.', | |||
| ); | |||
| invariant( | |||
There was a problem hiding this comment.
This wasn’t a hot path...
There was a problem hiding this comment.
Thanks, that part looks good
| @@ -678,6 +673,12 @@ class ReactDOMServerRenderer { | |||
| context: Object, | |||
| parentNamespace: string, | |||
| ): string { | |||
| invariant( | |||
There was a problem hiding this comment.
...but this is. Seems unfortunate to run this check every time.
(Also, if we do this I’d expect it to happen outside of renderDOM which renders host components only.)
Since we already read .type above when comparing to fragment maybe we should turn that branch into a switch?
packages/shared/getComponentName.js
Outdated
| function getComponentName(fiber: Fiber): string | null { | ||
| const {type} = fiber; | ||
| if (type === REACT_CALL_TYPE) { | ||
| return '<ReactCall>'; |
There was a problem hiding this comment.
Why didn’t Fragment need this?
There was a problem hiding this comment.
I guess because we don't have a test that covers them. I'll add one.
There was a problem hiding this comment.
Actually I guess the debug tool is supposed to skip over these
sebmarkbage
reviewed
Dec 12, 2017
| fiber.expirationTime = expirationTime; | ||
| return fiber; | ||
| default: { | ||
| if (typeof type === 'function') { |
There was a problem hiding this comment.
We should put these type checks first before the switch. Always make sure that the common path is first.
Don't think these were intentionally omitted from the blacklist of component types. I went ahead and updated getComponentName to include special types, even though I don't think they're used anywhere right now.
sebmarkbage
approved these changes
Dec 12, 2017
gaearon
reviewed
Dec 12, 2017
packages/shared/getComponentName.js
Outdated
| return type.displayName || type.name; | ||
| switch (type) { | ||
| case REACT_FRAGMENT_TYPE: | ||
| return '<ReactFragment>'; |
There was a problem hiding this comment.
Should this just be React.Fragment? Same for others. Some our warnings put < and /> around names.
yenshih
pushed a commit
to yenshih/react
that referenced
this pull request
Jan 6, 2018
* Call and Return components should use ReactElement ReactChildFiber contains lots of branches that do the same thing for different child types. We can unify them by having more child types be ReactElements. This requires that the `type` and `key` fields are sufficient to determine the identity of the child. The main benefit is decreased file size, especially as we add more component types, like context providers and consumers. This updates Call and Return components to use ReactElement. Portals are left alone for now because their identity includes the host instance. * Move server render invariant for call and return types * Sort ReactElement type checks by most likely * Performance timeline should skip over call components Don't think these were intentionally omitted from the blacklist of component types. I went ahead and updated getComponentName to include special types, even though I don't think they're used anywhere right now. * Remove surrounding brackets from internal display names
This was referenced Dec 18, 2020
This was referenced Sep 16, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ReactChildFiber contains lots of branches that do the same thing for different child types. We can unify them by having more child types be ReactElements. This requires that the
typeandkeyfields are sufficient to determine the identity of the child.The main benefit is decreased file size, especially as we add more component types, like context providers and consumers.
This updates Call and Return components to use ReactElement. Portals are left alone for now because their identity includes the host instance.