Refactor stack handling (no functional changes) #13165
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
This makes the difference between it and ReactFiberCurrentFrame a bit clearer. ReactDebugCurrentFiber is Fiber's own implementation. ReactFiberCurrentFrame is the thing that holds a reference to the current implementation and delegates to it.
Conceptually they're very related. ReactFiberComponentTreeHook contains implementation details of reading Fiber's stack (both in DEV and PROD). ReactDebugCurrentFiber contained a reference to the current fiber, and used the above utility. It was confusing when to use which one. Colocating them makes it clearer what you could do with each method. In the future, the plan is to stop using these methods explicitly in most places, and instead delegate to a warning system that includes stacks automatically. This change makes future refactorings simpler by colocating related logic.
gaearon
requested review from
sophiebits,
bvaughn,
flarnie,
acdlite and
sebmarkbage
Details of bundled changes.Comparing: 3596e40...6b625cc react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
gaearon
changed the title
gaearon
commented
Jul 6, 2018
| ReactDebugCurrentFiber.current = null; | ||
| ReactDebugCurrentFiber.phase = null; | ||
| export function resetCurrentFiber() { | ||
| if (__DEV__) { |
There was a problem hiding this comment.
Technically this DEV block is a change, but we could never get here in PROD — if we did, the next line would crash.
There was a problem hiding this comment.
Interesting - we do always check for 'DEV' before doing this, seems fine to inline the 'DEV' check.
Clarify which are DEV or PROD-only. Clarify which can return null. I believe the "work in progress only" was a mistake. I introduced it because I wasn't sure what guarantees we have around .return. But we know for sure that following a .return chain gives us an accurate stack even if we get into WIP trees because we don't have reparenting. So it's fine to relax that naming.
gaearon
force-pushed
the
refactor-some-stuff
branch
from
b4e6295
to
146ba8e
Compare
flarnie
approved these changes
Jul 6, 2018
It's not completely DEV-only anymore. Individual methods already specify whether they work in DEV or PROD in their names.
|
Sure enough, putting |
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.
1. Refactor
ReactDebugCurrentFiberto use named exports (919d734)This makes the difference between it and
ReactFiberCurrentFramea bit clearer.ReactDebugCurrentFiberis Fiber's own implementation.ReactFiberCurrentFrameis the "isomorphic" thing that holds a reference to the current implementation and delegates to it. Note: I’m not changingReactFiberCurrentFramein this PR.2. Unify
ReactFiberComponentTreeHookandReactDebugCurrentFiber(26e0b53)Conceptually they're very related.
ReactFiberComponentTreeHookcontains implementation details of reading Fiber's stack (both in DEV and PROD).ReactDebugCurrentFibercontained a reference to the current fiber, and usedReactFiberComponentTreeHookto actually get its stack.In the future, the plan is to stop using these methods explicitly in most places, and instead delegate to a warning system that includes stacks automatically.
3. Rename methods to better reflect their meanings (b4e6295)
Clarify which are DEV-only, and which can be used for PROD stacks. Clarify which can return null (I opted to do it via naming because we had several bugs where we assumed it's a string).
I believe the "work in progress only" was a mistake. I introduced it because I wasn't sure what guarantees we have around
.return. But we know for sure that following a.returnchain gives us an accurate stack even if we get into WIP trees because we don't have reparenting. So it's fine to relax that naming.This change makes future refactorings simpler by colocating related logic. We don't gain anything from having two modules (
ReactFiberComponentTreeHookandReactDebugCurrentFiber) that do almost exactly the same thing, but with different inputs. Its placement insharedwas also misleading because it's reconciler-specific. It was only insharedbecause in the past there was Stack-related code in this file.