[Fizz] Implement Component Stacks in DEV for warnings #21610
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
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 44cdfd6...3d2a1ab Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
gaearon
reviewed
Jun 3, 2021
| } | ||
| function popComponentStackInDEV(task: Task): void { | ||
| if (__DEV__) { | ||
| invariant( |
There was a problem hiding this comment.
I rationalized it because it would be a React bug if it ever happened, so it shouldn't happen. But yea it shouldn't happen conditionally even then. I'll change it.
gaearon
approved these changes
Jun 3, 2021
bvaughn
reviewed
Jun 3, 2021
| }; | ||
| }: any); | ||
| if (__DEV__) { | ||
| task.componentStack = currentTaskInDEV ? task.componentStack : null; |
There was a problem hiding this comment.
Why do you check currentTaskInDEV before reading from task?
There was a problem hiding this comment.
This looks like a mistake. You're assigning task.componentStack = task.componentStack
There was a problem hiding this comment.
Oops. Yea. Looks like I should add a test that tests component stacks when there's a suspended boundary inbetween.
bvaughn
reviewed
Jun 3, 2021
|
The Flight server renderer is more complicated because it uses JSON.stringify so we don't have access to what happens on the stack. We'd have to reimplement that for DEV or perhaps which technique in both PROD/DEV. There are context related reasons we might want to switch implementation away from JSON.stringify for that reason. |
Right 😬 but we will need something similar for DevTools. Right now, Flight server doesn't track either owner or parent. |
This uses a reverse linked list in DEV-only to keep track of where we're currently executing.
This makes it more explicit which stack we pass in to be retained by the task.
sebmarkbage
commented
Jun 3, 2021
| ); | ||
| } | ||
|
|
||
| // We can't use the toErrorDev helper here because this is an async act. |
There was a problem hiding this comment.
This one is fun. We can probably port it to make it work with async functions too.
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Jun 7, 2021
Summary: - **[0eea57724](facebook/react@0eea57724 )**: Fix typo in comment (accumlated → accumulated) ([#21637](facebook/react#21637)) //<ithinker5>// - **[0706162ba](facebook/react@0706162ba )**: Fix typo in comment (environement → environment) ([#21635](facebook/react#21635)) //<niexq>// - **[9d17b562b](facebook/react@9d17b562b )**: Fix typo in comment (satsify → satisfy) ([#21629](facebook/react#21629)) //<niexq>// - **[b610fec00](facebook/react@b610fec00 )**: fix comments: expiration time -> lanes ([#21551](facebook/react#21551)) //<Shannon Feng>// - **[cc4d24ab0](facebook/react@cc4d24ab0 )**: [Fizz] Always call flush() if it exists ([#21625](facebook/react#21625)) //<Dan Abramov>// - **[e0d9b2899](facebook/react@e0d9b2899 )**: [Fizz] Minor Fixes for Warning Parity ([#21618](facebook/react#21618)) //<Sebastian Markbåge>// - **[1b7b3592f](facebook/react@1b7b3592f )**: [Fizz] Implement Component Stacks in DEV for warnings ([#21610](facebook/react#21610)) //<Sebastian Markbåge>// - **[39f007489](facebook/react@39f007489 )**: Make enableSuspenseLayoutEffectSemantics static for www ([#21617](facebook/react#21617)) //<Brian Vaughn>// - **[8f3794276](facebook/react@8f3794276 )**: Prepare semver (`latest`) releases in CI ([#21615](facebook/react#21615)) //<Andrew Clark>// - **[8b4201535](facebook/react@8b4201535 )**: Devtools: add feature to trigger an error boundary ([#21583](facebook/react#21583)) //<Bao Pham>// - **[154a8cf32](facebook/react@154a8cf32 )**: Fix reference to wrong variable //<Andrew Clark>// - **[6736a38b9](facebook/react@6736a38b9 )**: Add single source of truth for package versions ([#21608](facebook/react#21608)) //<Andrew Clark>// - **[86715efa2](facebook/react@86715efa2 )**: Resolve the true entry point during tests ([#21505](facebook/react#21505)) //<Sebastian Markbåge>// - **[a8a4742f1](facebook/react@a8a4742f1 )**: Convert ES6/TypeScript/CoffeeScript Tests to createRoot + act ([#21598](facebook/react#21598)) //<Sebastian Markbåge>// - **[1d3558965](facebook/react@1d3558965 )**: Disable deferRenderPhaseUpdateToNextBatch by default ([#21605](facebook/react#21605)) //<Sebastian Markbåge>// - **[a8964649b](facebook/react@a8964649b )**: Delete an unused field ([#21415](facebook/react#21415)) //<okmttdhr>// - **[76f85b3e5](facebook/react@76f85b3e5 )**: Expose Fizz in stable builds ([#21602](facebook/react#21602)) //<Dan Abramov>// - **[e16d61c30](facebook/react@e16d61c30 )**: [Offscreen] Mount/unmount layout effects ([#21386](facebook/react#21386)) //<Andrew Clark>// - **[63091939b](facebook/react@63091939b )**: OSS feature flag updates ([#21597](facebook/react#21597)) //<Brian Vaughn>// - **[efbd69b27](facebook/react@efbd69b27 )**: Define global __WWW__ = true flag during www tests ([#21504](facebook/react#21504)) //<Sebastian Markbåge>// - **[28625c6f4](facebook/react@28625c6f4 )**: Disable strict effects for legacy roots (again) ([#21591](facebook/react#21591)) //<Brian Vaughn>// - **[3c2341416](facebook/react@3c2341416 )**: Update jest to v26 ([#21574](facebook/react#21574)) //<Sebastian Silbermann>// - **[0d493dcda](facebook/react@0d493dcda )**: Removed _debugID field from Fiber - Issue #21558 ([#21570](facebook/react#21570)) //<Pulkit Sharma>// - **[7841d0695](facebook/react@7841d0695 )**: Enable the updater-tracking feature flag in more builds ([#21567](facebook/react#21567)) //<Brian Vaughn>// - **[6405efc36](facebook/react@6405efc36 )**: Enabled Profiling feature flags for OSS release ([#21565](facebook/react#21565)) //<Brian Vaughn>// Changelog: [General][Changed] - React Native sync for revisions 2d8d133...0eea577 jest_e2e[run_all_tests] Reviewed By: bvaughn Differential Revision: D28932083 fbshipit-source-id: 012c1bfb857ed59d7283334d633f1cce8ec50360
koto
pushed a commit
to koto/react
that referenced
this pull request
Jun 15, 2021
* Implement component stacks This uses a reverse linked list in DEV-only to keep track of where we're currently executing. * Fix bug that wasn't picking up the right stack at suspended boundaries This makes it more explicit which stack we pass in to be retained by the task.
sebmarkbage
added a commit
that referenced
this pull request
Jul 4, 2024
…ing onError/onPostpone (#30206) Stacked on #30197. This is similar to #30182 and #21610 in Fizz. Track the current owner/stack/task on the task. This tracks it for attribution when serializing child properties. This lets us provide the right owner and createTask when we console.error from inside Flight itself. This also affects the way we print those logs on the client since we need the owner and stack. Now console.errors that originate on the server gets the right stack on the client: <img width="760" alt="Screenshot 2024-07-03 at 6 03 13 PM" src="https://github.com/facebook/react/assets/63648/913300f8-f364-4e66-a19d-362e8d776c64"> Unfortunately, because we don't track the stack we never pop it so it'll keep tracking for serializing sibling properties. We rely on "children" typically being the last property in the common case anyway. However, this can lead to wrong attribution in some cases where the invalid property is a next property (without a wrapping element) and there's a previous element that doesn't. E.g. `<ClientComponent title={<div />} invalid={nonSerializable} />` would use the div as the attribution instead of ClientComponent. I also wrap all of our own console.error, onError and onPostpone in the context of the parent component. It's annoying to have to remember to do this though. We could always wrap the whole rendering in such as context but it would add more overhead since this rarely actually happens. It might make sense to track the whole current task instead to lower the overhead. That's what we do in Fizz. We'd still have to remember to restore the debug task though. I realize now Fizz doesn't do that neither so the debug task isn't wrapping the console.errors that Fizz itself logs. There's something off about that Flight and Fizz implementations don't perfectly align.
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.
This uses a reverse linked list in DEV-only to keep track of where we're currently executing. It works similar to context in that it saves these on the running task.
In addition to this we need access to the current task to extract it. We could keep it only the "current component stack" but we still need to be able to stash it on the task occasionally.
This causes a bit of an awkwardness where current task is available and also the task is passed as an argument to all current functions.
Note that I wasn't planning on using this for production stacks. We don't invoke error boundaries anyway but we could pass stacks to the global callback. But this seems a bit heavy just for that. A variant of this would be to use the plain stack but that doesn't carry along the stack between tasks. A compromise could be to only collect the stack on the current running stack. This is effectively what happens with Promises in production since async boundaries aren't tracked there.