-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
React.warn() and React.error() #15170
Conversation
React: size: 🔺+0.3%, gzip: 🔺+0.2% Details of bundled changes.Comparing: 56035da...625aaf6 react
Generated by 🚫 dangerJS |
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 is going to be so good for open source land. LGTM.
Follow up:
|
257541f
to
1078cc9
Compare
Okay, PR has been updated~ |
Why make it a no-op in production, vs logging the message sans stack? |
To gently encourage people to use this API for DEV-only messaging. |
This makes library life so much easier 🙏 Could we change the signature to be equal to existing APIs such as In it's current form - warning(someInvariant, message)
+ React.warn(someInvariant, message) |
Tbh our experience with |
I've personally always found those signatures confusing. I think these methods being 1:1 with their |
Also to be clear, the idea is for these to be DEV-only warnings. So you would want some form of |
100% agreed here. I'm never comfortable once I have to negate boolean logic. To many moving parts.
I would disagree here looking at the download stats for
That's where - import warning from 'warning';
+ import { warn as warning } from 'react'; if the signatures were the same. But I understand if you want to use the opportunity to create a better API. We can still wrap them with a custom function that has the same API as function warning(condition, ...reactWarnArgs) {
if (!condition) {
React.warn(...reactWarnArgs)
}
} |
A lot of this was just cargo cult from early on. I and other people saw React using |
I think wrapping (if you want to mimic the |
if (__DEV__) { | ||
const ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame; | ||
|
||
error = (...args) => { |
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.
Let's write out the ES5 code. I don't trust the Babel plugin to do a better job than we do. (And we can easily accidentally forget to transpile it and cause more trouble downstream.)
@eps1lon Maybe we should deprecate the babel plugin? If React is making the if branch explicit, adding an extra import { warn } from "react";
function List(props) {
if ((props.innerTagName != null || props.outerTagName != null) && process.env.NODE_ENV !== "production") {
warn(
"The innerTagName and outerTagName props have been deprecated. " +
"Please use the innerElementType and outerElementType props instead."
);
}
// ...
} |
Wouldn't the result end up being something more like this? import { warn } from "react";
function List(props) {
// This check wouldn't be removed, so it would bloat the bundle.
if (props.innerTagName != null || props.outerTagName != null) {
if (process.env.NODE_ENV !== "production") {
warn(
"The innerTagName and outerTagName props have been deprecated. " +
"Please use the innerElementType and outerElementType props instead."
);
}
}
// ...
} |
Env check should always be the first thing in the if statement - because as @bvaughn has noticed other conditions might not get removed properly if you put the env check non-first. In this case minifier wont assume in most cases that getters, such as props.innerTagName here, are side-effect free. |
Will this print the error/warn multiple times if the component is re-rendered? |
@diegohaz looking at the implementation - yes, it's just a wrapper around console.warn/error: |
Yes. Up to you to do your own de-duping. I suggest using a |
Would this be slated for 16.9? Looks great! |
Yeah planned for 16.9 |
Awesome! I'm very excited to finally have something here. Did you consider including the "welcome to debugging in react" throw/catch for |
No, I don't think was considered, since we're just calling
This was considered briefly and TBH I don't remember the discussion super clearly. I think we wanted to avoid an API that might encourage people parsing the stack and building things on top of it. (You can obviously still intercept |
Library authors often request a way to log warnings that include the React "component stack". This PR adds two new top-level APIs to for this purpose:
React.warn
andReact.error
These methods avoid exposing the stack directly so that people aren't tempted to parse it. (They can obviously still do this by intercepting calls to
console.error
but at least the API doesn't encourage it.) Instead they wrapconsole.error
andconsole.warn
and append the component stack when available.Usage
Example usage (borrowed from react-window):
Which might log something like:
Behavior
The behavior of these new methods is as follows:
console
method?