-
Notifications
You must be signed in to change notification settings - Fork 46.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Build Component Stacks from Native Stack Frames #18561
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit fbb0189:
|
Details of bundled changes.Comparing: af1b039...fbb0189 react-art
react-test-renderer
react
react-native-renderer
react-dom
react-reconciler
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
Details of bundled changes.Comparing: af1b039...fbb0189 react
react-dom
react-art
react-test-renderer
react-native-renderer
react-reconciler
Size changes (experimental) |
97fac34
to
d7a16ff
Compare
d7a16ff
to
a2aa270
Compare
ReactDebugCurrentFrame.setCurrentlyValidatingElement = function( | ||
element: null | ReactElement, | ||
) { | ||
ReactDebugCurrentFrame.setExtraStackFrame = function(stack: null | string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit could just assign:
ReactDebugCurrentFrame.setExtraStackFrame = setExtraStackFrame;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's DEV it probably doesn't matter but in general that might not be great because one is meant to be called in this same package so it can be inlined. So that would avoid inlining it.
@@ -7,26 +7,20 @@ | |||
* @flow | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The organization of this file is confusing to me.
- We unconditionally export a setter which sets a variable that is not exported (and has no getter or is otherwise unused except for DEV mode which uses it in
getStackAddendum
). - We assign a duplicate of that same setter to
ReactDebugCurrentFrame
in DEV mode, but we don't assign a getter. We leave that up to the current renderer, which of course, doesn't even use the local value written byReactDebugCurrentFrame.setExtraStackFrame
because it has no way to access it. So instead it uses its own local current fiber value.
Looks like the only thing using the exported setCurrentlyValidatingElement
function (now setExtraStackFrame
) is ReactElementValidator
- and even it only uses it in DEV. So why are we using this odd pattern?
I'm not super familiar with some of these files so maybe there's a subtle difference that I'm overlooking.
Suggestion: Update ReactElementValidator
to use ReactDebugCurrentFrame.*
too, then get rid of the ES export.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's complicated because this is used by jsx-runtime
too which is a separate file that needs to import this from isomorphic. This needs some further refactoring in a follow up at some point.
For now I'm just removing a cycle that otherwise would break.
if (__DEV__) { | ||
setCurrentlyValidatingElement(element); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I noticed this while writing my above comment.
// $FlowFixMe Flow thinks console is immutable. | ||
console.error = prevError; | ||
/* eslint-enable react-internal/no-production-logging */ | ||
disabledDepth--; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we throw or warn if we ever go < 0?
|
||
describe('Component stack trace displaying', () => { | ||
beforeEach(() => { | ||
React = require('react'); | ||
ReactDOM = require('react-dom'); | ||
}); | ||
|
||
if (ReactFeatureFlags.enableComponentStackLocations) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look necessary (at least not yet?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll just remove this test later. It's not very useful for this model. This test fails with the stack on, partly because the warning gets deduped because it's always the same component and line being called.
// V8 adds a "new" prefix for native classes. Let's remove it to make it prettier. | ||
const frame = '\n' + sampleLines[s - 1].replace(' at new ', ' at '); | ||
if (__DEV__) { | ||
if (typeof fn === 'function') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this typeof
check necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it throws in WeakMap if the wrong type sneaks in. That kills the stack which is not good if we're warning because some wrong type. We have tests that cover that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. It looked odd to have fn: Function
followed by a typeof
check.
In DEV it's somewhat likely that we'll see many logs that add component stacks. This could be slow so we cache the results of previous components.
In V8 we need to ignore the first line. Normally we would never get there because the stacks would differ before that, but the stacks are the same if we end up throwing at the same place as the control.
This drops the requirement to include owner to pass the test.
This ensures that we don't double call yieldValue or advanceTime in tests. Ideally we could use empty destructuring but ES lint doesn't like it.
f469cbe
to
fbb0189
Compare
Instead of using displayName/name and source location from jsxDEV, this uses a hack to extract the native stack frame of the function. That way it's zero-cost when an error is not hit and we can feel good about including it in production environments too.
It works by calling each function once with invalid input and intentionally causing it to throw and catching the error. This should be ok since they shouldn't have side-effects.
By doing so we get the stack frame of that function somewhere in the error's stack. We then compare it to a control stack to figure out which one is the next frame after that. This should be the frame of the component. We use that to build a stack.
This stack should be parsable by off-the-shelf cross-browser stack parsing libraries.
The idea is to throw as early as possible. Ideally it's the first line such as the built-in super call of a class or the destructuring of props in the argument position. It could however be slightly later which might be slightly confusing to see the line of the first hook for example. Source code aware tooling could adjust this number to the previous function definition before the offset. If it doesn't throw at all such as if no hooks and no props are used, then we can't figure out the line number.
This doesn't give us the offset of the JSX call so it's slightly worse than the source info transform in that regard. However, it has the benefit of yielding the client-side source position rather than file name source position. This allows linkifying the location on the client so you can jump to source. It does require source maps to do so but it can piggyback on standard error logging strategies. This PR make the flag take over both DEV and prod so the source location from the transform is not used for consistency.
If this ends up being considered worse than the JSX transform due to the lack of JSX callsites, I have another idea to use the second error recovery pass in DEV to collect exact JSX callsites using stack frames but that's way more complicated and code. We wouldn't ship that to prod anyway.
TODO: