RFC: Warning System Revamp

[gaearon]

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. warnings are conceptually errors, and use console.error, but their name doesn't reflect that.
• The warning(cond, ...) API is confusing. It is easy to forget whether cond is supposed to be true or false for the warning to fire. (Answer: it fires on false.) Due to this confusion, a bunch of callsites just do if (!cond) warning(false, ...) to avoid thinking about this.
• warning and lowPriorityWarning have different default behavior. warning appends component stack by default, but lowPriorityWarning doesn't. This makes it difficult to "downgrade" a warning to lowPriorityWarning because we'd lose the stack and have to manually append it.
• Some warnings 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) and warn (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), and console.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.

How Do We Get There?

Step 1. lowPriorityWarning() Parity

• Rename lowPriorityWarning to lowPriorityWarningWithoutStack.
• 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.

[bvaughn]

There's some small overlap between this and DevTools own automatic appending of component stacks. I don't expect the proposed changes here will interfere with that, but it's something we should just keep in the back of our mind in case. 😄

Also as we're doing this overhaul, if any ideas come up for new ways DevTools could better help with warnings- let's talk!

[acdlite]

@Jessidhia Glad you're working on this! Maybe when you're done you could pick up #15072, too 😆

[jordanstephensen]

Is there any documentation on the lowPriorityWarning module? I'm having a hard time finding any.

[gaearon]

We’ve done all of this except the “warning audit”. I’ll close.