Move traverseFragmentInstanceChildren to internal ReactFiberTreeReflection #32613
Conversation
This file contains hidden or 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
github-actions
bot
added
the
React Core Team
rickhanlonii
approved these changes
Mar 14, 2025
jackpope
approved these changes
Mar 14, 2025
Contributor
jackpope
left a comment
jackpope
left a comment
There was a problem hiding this comment.
Make sense thanks for the refactor and explanation
github-actions bot
pushed a commit
that referenced
this pull request
Mar 14, 2025
…ction (#32613) This is a nit but a Config should not have to know anything about the internals of Fibers. Ideally it shouldn't even access them but we have some cases where we need pointers back in like for this fragment. The way we've typically abstracted this is using the `ReactFiberTreeReflection` helper that's in the `react-reconciler`. Such as in the event system. https://github.com/facebook/react/blob/f3c956006a90dc68210bd3e19497d10fb9b028d3/packages/react-dom-bindings/src/events/ReactDOMEventListener.js#L22-L26 We sometimes cheat but we really should clean this up such that a `Fiber` is actually an opaque type to the Configs and it can never dot into it without using a helper. So this just moves `traverseFragmentInstanceChildren` to ReactFiberTreeReflection so that the ConfigDOM doesn't ever dot into its fields itself. It just passes the Fiber through back into the react-reconciler. I had to add a wrapper to read the `.child` to avoid that being assumed too. I also noticed that FragmentInstanceType is not actually passed through so that argument is unnecessary. DiffTrain build for [6b5d9fd](6b5d9fd)
This was referenced Mar 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
This is a nit but a Config should not have to know anything about the internals of Fibers. Ideally it shouldn't even access them but we have some cases where we need pointers back in like for this fragment.
The way we've typically abstracted this is using the
ReactFiberTreeReflectionhelper that's in thereact-reconciler. Such as in the event system.react/packages/react-dom-bindings/src/events/ReactDOMEventListener.js
Lines 22 to 26 in f3c9560
We sometimes cheat but we really should clean this up such that a
Fiberis actually an opaque type to the Configs and it can never dot into it without using a helper.So this just moves
traverseFragmentInstanceChildrento ReactFiberTreeReflection so that the ConfigDOM doesn't ever dot into its fields itself. It just passes the Fiber through back into the react-reconciler. I had to add a wrapper to read the.childto avoid that being assumed too. I also noticed that FragmentInstanceType is not actually passed through so that argument is unnecessary.