-
Notifications
You must be signed in to change notification settings - Fork 46k
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
Replace React.error and React.warn with getComponentStack #16017
Conversation
import ReactSharedInternals from 'shared/ReactSharedInternals'; | ||
|
||
let getComponentStack = function() { | ||
return ''; |
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 this not return anything? Seemed weird to return void/undefined in prod and an empty string when called outside of the render phase in DEV, so I opted to return empty string in both cases for now.
React: size: 🔺+0.1%, gzip: -0.1% Details of bundled changes.Comparing: eb2ace1...cd2fc91 react
Generated by 🚫 dangerJS |
It’s not just about the component stack but other things we can do with it like reorder and delay etc. This doesn’t address the abuse problem. The reason this wasn’t earlier is because there were many really bad use cases people were intending to use this for. |
That sounds more like a broader logging strategy which we still need to decide on (#15726). I see that as separate from the more focused need of accessing a component stack. Do you disagree?
TBH bad actors would probably misuse e.g. |
Sebastian and I had a productive conversation this afternoon regarding my concerns above. The quick takeaway is this: I will be updating this PR to remove In the meanwhile, I will be adding functionality to React DevTools to intercept calls to Might also be worth looking into a log view within DevTools that groups and orders logs by commit, etc. |
Closing in favor of #16126 |
Proposed alternative to #15170
Several people have asked, "Could you provide a method to access the component stack directly?". I think our reasoning against this was that we didn't want people to depend on that stack, or to try to parse it and build anything on top of it.
I don't think we considered the case of instrumenting error logging though. Facebook (and probably others) overrides
console.error
to capture the call stack and save it somewhere. A downside ofReact.error
is that it would cause errors from different call sites to be grouped together (unless the instrumenting code was smart enough to skip over the React frame).Overriding only these two console methods also feels like a bit of an incomplete solution (#15726) to the logging story.
This PR replaces
React.warn
andReact.error
with a method that returns the current component stack¹ which people can log to whichever console group they want.¹ At the moment, this method always returns an empty string in prod.