Fix inspect stack not returning all ancestors #693
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Please see my inline comments. I think it'd be good to test this on RN76 on both Android and iOS, as this is the only project that we have that's currently setup with the new architecture where some of the internals that you're accessing here may work differently.
|
I tested this on rn76 (both Android and iOS), it looks like it's working 😄 |
There was a problem hiding this comment.
This PR seems to refactor some bits of the old codebase w/o qestioning why they've been implemented in the certain way. Specifically:
- inline requies
- try catch around measure
- Promise.all approach
The use of promises here doesn't seem to make a lot of sense. Before the role of promises was such that we can call async measure in parallel. This PR changes it and makes all measure calls sequential and thus potentially slower.
|
@kmagiera As for the I also changed the recursive procedure of traversing the tree into an iterative one, also could be a bit faster |
There was a problem hiding this comment.
Left some more inline comments in the code.
In general what worries me is that I don't understand what this PR is actually fixing. It links to an empty issue but there are no reproducable steps that would allow us to test before/after this change and see if it fixes anything. This is important as in the future, if someone wants to change something in this code, they may want to test again the scenario that this PR claims to fix.
| height: viewHeight | ||
| }; | ||
|
|
||
| stackElement = (inspectorData.source) ? |
There was a problem hiding this comment.
| stackElement = (inspectorData.source) ? | |
| const stackElement = (inspectorData.source) ? |
There was a problem hiding this comment.
Also, it appears like we can tell whether source is set before we call measure, so maybe we can skip calling measure altogether in such a case?
There was a problem hiding this comment.
added an if statement before measure to check if source is absent and resolve the promise early in such case
|
Will re-test again after changes and try to land this |
Changes
Fixes #692
This PR is also necessary for #666
This PR aims to fix the issue with inspector stack that not all ancestors of inspected elements are discovered. It turns out that the hierarchy returned by
getInspectorData()is already missing these ancestors, so we have to traverse the component tree ourselves.The first node is the
closestInstancein object returned bygetInspectorDataViewAtPoint. To access parent node of a node, we look atnode.return. This way we can simply traverse the whole path from first node up to the root. Along the way, we collect all necessary information and create a stack that is returned at the end. We usegetInspectorDataForInstance(node)renderer function to obtain inspector data for given fiber node.This version is a lot more accurate, from my testing it returns all the most used visible components (like
<View>,<Text>,<Button>), which is already an improvement compared to the current solution.Demos
The following are demos how the inspect stack looks like before and after changes from this PR. The component hierarchy of the app consists of several nested components, which have border for visualization.
Before
better_stack_demo_before.mov
After
better_stack_demo_after.mov
How Has This Been Tested:
Open test app, use inspector tool, use right click to show inspector stack.
Tested on Android:
Test on iOS: