[Fiber][WIP] Include component stack in Fiber #8562
Conversation
| } = ReactTypeOfSideEffect; | ||
|
|
||
| function coerceRef(current: ?Fiber, element: ReactElement<any>) { | ||
| function coerceRef(current: ?Fiber, element: ReactElement) { |
There was a problem hiding this comment.
Because I imported our internal ReactElement definition that is not generic.
|
|
||
| }; | ||
|
|
||
| export type FiberDev = Fiber & { |
There was a problem hiding this comment.
Having two types is not sustainable with all the different variants we need and it has little benefit since you have to coerce it anyway. The coercion is worse than just assuming it is always available since it will drop information about which specific Fiber it is if we overload it.
In #8545 I simply added this field to all Fibers.
So I'd suggest just doing the same. Add it to the Fiber.
There was a problem hiding this comment.
Btw, I had to fix this pattern when I did that: https://github.com/facebook/react/pull/8545/files#diff-5842acf7560d3c499ac59f2652b3918dR328
There was a problem hiding this comment.
If I add it to the Fiber how can I convince Flow it exists even if I don't set it in DEV? Do the coercion when returning?
There was a problem hiding this comment.
There was a problem hiding this comment.
Yea. I think we'll want to change the constructor logic there at some point to pass everything that can be non-nullable to satisfy the use case in #8545 but we probably need to trick Flow to follow it regardless. Perhaps we can use declare __DEV__ : true; somewhere to trick Flow into always taking that path (although that might trigger unused paths and miss errors in those paths).
| const fiberDev = ((fiber : any) : FiberDev); | ||
| if (tag === HostRoot) { | ||
| // Roots are unobservable for debug tools | ||
| fiberDev._debugID = 0; |
There was a problem hiding this comment.
I think we should actually expose the roots in the debug tools since we might want to expose info about the container. Additionally, since roots can contain fragments there can actually be multiple roots if we were to ignore the container. At least add a TODO here.
There was a problem hiding this comment.
Yes. We'll also want to expose portals but I want to get to parity first.
|
So what's the plan for handling incremental and errors? Since these mounts can be terminated, restarted and resumed you can end with all kinds of inconsistent states. At the very least this will cause leaks because Would it be better to track changes in the commit phase? |
|
I haven't really thought about this yet. For stacks incrementalness shouldn't matter since we only use the immutable data (type and parent). For other cases, dunno, I’ll figure something out when I get closer to other cases. |
|
At the very least I'm hesitant to land this while it is leaking. |
|
For sure, me too. I put up the PR mostly for discussing Flow. I’ll definitely want to solve leaks somehow. |
|
Closing in favor of #8570. |
3 participants
This gets some tests passing.
Still needs work—for example I never clean up that data right now.
I want to get feedback on initial approach.
Do Flow hacks make sense here?