Description
This is a proposal to change how the internal warning system works.
Note: @walaura is already working on this, please don't send PRs.
Current System
React has a concept of "warnings". Conceptually, most of them should be treated as errors. They indicate bugs. For example, not fixing a "key" warning can result in very bad issues in production. For these "warnings", the only difference from a real error is that they don't throw and the checks are removed in production. Because they're expensive to do.
React warnings ultimately become console.error
calls. In the source, they are expressed as warning(cond, message, ...args)
. If the cond
is false, the warning gets printed.
By default, calling warning
will print a console.error
with the current component stack appended at the end. Sometimes, we may not want the component stack. Maybe the warning is aggregated from many components (e.g. a StrictMode violation), and the stack is not relevant. In that case we have warningWithoutStack
. It has the same API as warning
, but doesn't append the stack. In fact, warning(format, ...args)
is internally implemented as warningWithoutStack(format + '%s', ...args, stack)
.
We also have a lesser-known lowPriorityWarning
module. Unlike warning
which uses console.error
, lowPriorityWarning
uses console.warn
. It is "lower severity" (appears yellow in console), and we currently use it only for deprecation messages.
Problems
- The naming is confusing.
warning
s are conceptually errors, and useconsole.error
, but their name doesn't reflect that. - The
warning(cond, ...)
API is confusing. It is easy to forget whethercond
is supposed to betrue
orfalse
for the warning to fire. (Answer: it fires onfalse
.) Due to this confusion, a bunch of callsites just doif (!cond) warning(false, ...)
to avoid thinking about this. warning
andlowPriorityWarning
have different default behavior.warning
appends component stack by default, butlowPriorityWarning
doesn't. This makes it difficult to "downgrade" awarning
tolowPriorityWarning
because we'd lose the stack and have to manually append it.- Some
warning
s don't actually represent "errors" in practice. They have too high severity. It's not a huge deal in the console. But if you start hooking up the console to richer mechanisms (e.g. an error dialog), the difference becomes more annoying. You want to clearly separate what's broken today from what may break tomorrow.
Ideal End State
- All React warnings are audited and split into two severities:
error
(stuff that is likely broken today) andwarn
(stuff that may break in the future).- This means we'll likely "downgrade" some mostly advisory warnings.
- React codebase just calls
console.error
for severe warnings (potential bugs), andconsole.warn
for mild warnings (e.g. deprecations).- A build step may wrap them in
__DEV__
blocks, append the component stack, and otherwise tweak the implementation.
- A build step may wrap them in
How Do We Get There?
Step 1. lowPriorityWarning() Parity
- Rename
lowPriorityWarning
tolowPriorityWarningWithoutStack
. - Add
lowPriorityWarning
which appends the stack (but don't add usages of it).
The goal here is just to make it easy to switch between warning
<-> lowPriorityWarning
or warningWithoutStack
<-> lowPriorityWarningWithoutStack
whenever we want.
Ensure toWarnDev
and toLowPriorityWarnDev
matchers are equivalent too and both support withoutStack
named argument.
Notice there's lowPriorityWarning.www.js
fork. It should be renamed to lowPriorityWarningWithoutStack.www.js
, but it should keep reqiure-ing lowPriorityWarning
inside (because it refers to an external module).
Step 1.5. Replace Babel plugin with an ESLint plugin
See #17081 (comment).
Step 2. Remove the condition argument
Write a codemod to convert all warning(cond, format, ...args)
(and its lowPriority*
or *WithoutStack
variations) to if (!cond) { warning(format, ...args) }
. If cond
is already false
, just omit the condition. You could do this manually but it seems error-prone. So I recommend a codemod.
You'll notice there are some "forks" of warning
, like warningWithoutStack.www.js
or lowPriorityWarning.www.js
. You'll want to modify them to reflect the new API — but keep in mind that external files they reference still have the old API. So they need to "translate" it.
Don't forget there are Babel plugins and build scripts that deal with warning
. They probably make assumptions about its argument order that you will need to consider. Make sure we don't emit invalid code or accidentally stop transforming them.
### Step 3. Renames
My proposal:
warning
=>consoleError
warningWithoutStack
=>consoleErrorNoStack
lowPriorityWarning
=>consoleWarn
lowPriorityWarningWithoutStack
=>consoleWarnNoStack
toWarnDev
=>toConsoleErrorDev
toLowPriorityWarnDev
=>toConsoleWarnDev
{withoutStack}
=>{noStack}
Make sure all build transforms, the warning extraction script, and the forks continue working.
Step 4. Warning Audit
Split consoleError
calls into two groups:
- Actual likely bugs today (e.g. missing key, or an UNSAFE method in concurrent mode)
- Possible future issues that don't cause bugs today (e.g. UNSAFE method in strict mode, or setState in unmounted component)
Downgrade the second group to consoleWarn
calls.
Follow-ups
At this point we'll be pretty close to direct console.error
calls. I don't know if we want to actually start doing that in the source, and have transform catch that. If we do, we'd need to find a way to express "no stack" in some other way (or even always append them).
If we stick with consoleError
imports, we might want to add a lint rule that prevents adding direct console.error
calls except a few places where it's intentional. So that we don't mix them up.
How to Split Work
This is gonna touch a lot of files. Expect merge conflicts etc.
I suggest splitting it like this:
- Step 1 as a PR. We can land this fast.
- Step 2 + Step 3 can be done as changes to build scripts + a codemod. It should be easy to re-run just the codemod. This is one PR.
- Then Step 4 after previous PR lands.
I need to emphasize again that @Jessidhia is taking this so please don't send PRs.