facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.3k stars 46.36k forks source link

[DevTools Bug]: Warnings are too "loud", mislabeled and make console difficult to use #25447

Open Sequoia opened 1 year ago

Sequoia commented 1 year ago

Website or app

https://codesandbox.io/s/purple-moon-ts7xzs?file=/src/App.js

Repro steps

It's not causing this in the codesandbox probably due to some flag missing, but locally in dev I've been getting the following when a stray prop is passed to a dom elem as an attribute (NB this output is truncated, this isn't even the whole message):

Screen Shot 2022-10-07 at 10 39 51 AM

Problems with the current logging approach

  1. This floods the console and pushes all other messages out of screen, making debugging difficult. This is the main issue.
  2. The severity/"loudness" of this log message is out of proportion to the issue. This is a fairly minor issue as it typically does not actually break anything, yet this log drowns out actual issues that I need to see more urgently
  3. This issue's importance is being misclassified & the wrong logging API used: console.error is being misused to log a warning. The purpose of different log levels is to allow the consumer (developer) to enable or disable logging of less important messages depending on their needs. Putting warnings in the "error" stream takes this control away from developers. When I am cleaning up upgrade issues, minor bugs etc. I will turn on warnings and see this, but when I'm trying to figure out why my GQL endpoints are erroring, I should be able to turn this off.

Ask

My goal is to stop this effectively "breaking" the error console, i.e. making it unusable by flooding it with messages. Possible approaches:

  1. move these warnings to console.warn, putting control back in the developer/consumer's hands
  2. flag to disable the stack traces so messages aren't so enormous
  3. use console.groupCollapsed to collapse these, so the stack is there but doesn't flood the console

FAQ

Why don't you just fix the errors? If you fix the errors, this isn't an issue. The solution is fix the errors.

I don't mean to brag, but I work on applications with lots of errors. I truly wish I could fix every single one, but I must pick my battles and this often means letting smaller issues slide in order to focus on bigger ones. Furthermore, when working on a shared application, it can be out of your power to fix all the errors. There are several reasonable reasons why someone would want to work on their application and ignore certain errors, at least temporarily.

In any event, "this is a valid error message so why should it be quieter" doesn't address the question of proportionality. Should a warning like this overtake the whole console? Should it use window.alert? Should it bail out and crash the whole application? It's clear that these approaches to alerting the developer/consumer to an issue are not proportional with the severity of the issue itself, and these more drastic approaches would be inappropriate, even if they "draw the developer/user's attention to the issue" as this clearly does. Not every issue is p0 which is why we have log levels, and in this context different log streams (info, warning, and error).

How often does this bug happen?

Every time

DevTools package (automated)

No response

DevTools version (automated)

No response

Error message (automated)

No response

Error call stack (automated)

No response

Error component stack (automated)

No response

GitHub query string (automated)

No response

Sequoia commented 1 year ago

related issues:

It's my view this issue is still active, not resolved.

neuodev commented 1 year ago

@Sequoia What makes the error messages so long is not the error message itself! it is the error stack which means you have a lot of nested components! This also explains why in your example here you only see a smaller error message as there is only 2 component!

Sequoia commented 1 year ago

Another example of this issue:

Screen Shot 2022-11-08 at 10 19 45 AM

What makes it even worse is for some reason I cannot collapse this message. Attempting to collapse the error message does nothing.

React is breaking my console. ☹️ Please fix this. Not everything is a five-alarm error, and even those things that are should not make the console unusable like this.

Sequoia commented 1 year ago

@AhmedIbrahim336 Thank you for the additional info. My complaint still stands: even with many nested components warning should not be logged as errors & they should not make the console unusable.

Having deeply nested component structures like this is not uncommon in large production React apps. Mine is not an edge case.

stefoid commented 1 year ago

@AhmedIbrahim336 Thank you for the additional info. My complaint still stands: even with many nested components warning should not be logged as errors & they should not make the console unusable.

Having deeply nested component structures like this is not uncommon in large production React apps. Mine is not an edge case.

@Sequoia hey you are dead right. I use intellij and it has a 'grep console' and to avoid this filling with nested garbage, you can tell it to fold any line that matches a pattern, and in this case, folding any line that starts with two spaces seems to do the trick - you get the warning in the console, but the rest of the stufff after it is folded away until you click on it. Maybe you can use a similar filter

Screen Shot 2023-01-13 at 2 03 28 pm

^^ reduces 105 lines to 5 lines without hiding the actual error. In fact it probably makes the warning more clear

IvanD4RE commented 1 year ago

@Sequoia just some juicy code to go around the problem until a real solution:

console.originalError = console.error;
console.error = function (...params) {
  const message=params.shift();
  const consoleError =message.startsWith("Warning: ")
      ?console.warn
      :console.originalError;
  consoleError(message, ...params);
};
dbalatero commented 1 year ago

@IvanD4RE I evolved your solution a bit, and decided to stick with console.error so that the errors would show up by default in the console. However, I include some hidden text in the error so you can filter out the errors with -ReactDOMWarning in the devtools console filter section.

As long as this issue is open, this code snippet should at least be a quality of life improvement, and provide a bit of filtering control:

// Development React warnings in the console are really loud and it's hard to
// see the actual logs you care about.
//
// This patches `console.error` to detect and reformat the logs.
//
// This should not show up in production builds.
if (process.env.NODE_ENV === 'development') {
  const consoleError = console.error;

  // Add a hidden filter in the output so that we can easily filter out the log
  // messages with the negative "-ReactDOMWarning" filter.
  const hiddenFilter = 'ReactDOMWarning';
  const hiddenStyles = 'color: transparent; font-size: 0px; user-select: none';

  console.error = (...args: Parameters<typeof consoleError>) => {
    // Fallback to normal console error unless this error came from react-dom.
    const trace = new Error().stack;
    if (!trace || !trace.includes('react-dom.development.js')) {
      return consoleError(...args);
    }

    // All react-dom warnings start with "Warning:"
    const firstArg = args[0];
    if (typeof firstArg !== 'string' || !firstArg.startsWith('Warning:')) {
      return consoleError(...args);
    }

    // If we get here, we're reasonably sure that it's a ReactDOM warning.
    const template = args.shift()?.replace(/%s$/, '');
    const stacktrace = args.pop();

    console.groupCollapsed(
      `%c⚠️ ${template}%c${hiddenFilter}`,
      'color: red; font-weight: normal',
      ...args,
      hiddenStyles
    );
    console.log(
      `Tip: Add %c-${hiddenFilter}%c to the log Filter box to silence these ReactDOM error groups%c%{hiddenFilter}`,
      'font-weight: bold',
      'font-weight: normal',
      hiddenStyles
    );
    consoleError(`%s%c${hiddenFilter}`, stacktrace, hiddenStyles);
    console.groupEnd();
  };
}
dbalatero commented 1 year ago

What makes the error messages so long is not the error message itself! it is the error stack which means you have a lot of nested components! This also explains why in your example here you only see a smaller error message as there is only 2 component!

Also I really disagree with this comment. Yes, in practice it would be nice to not have a lot of nested components, but out here in the real world this is never the case. For any reasonably complex React app (which many of us work on at day jobs), there's no getting around a deep component hierarchy.

NullEnt1ty commented 5 months ago

Are there any updates on this?

dbalatero commented 5 months ago

Also is a PR welcome to improve this situation? As a first step, I'd love to have a chat with any maintainer here and figure out what acceptable solutions to this would be accepted. I don't know how to summon a maintainer though!

Some options:

  1. Provide a way to register warning filter functions that could be ran right before printing a message in development. Something like:
import { registerWarningFilter } from 'react-dom';

if (process.env.NODE_ENV === 'development') {
  const filterWarning = (error) => {
    const ignoredPackages = ['underscore', 'some-other-package-that-is-horribly-out-of-date-and-noisy'];

    if (error.message.includes("some phrase I want to match on")) {
      // suppress the error
      return true;
    }

    if (ignoredPackages.some((package) => error.stack.some((line) => line.includes(package)))) {
      return true;
    }

    return false;
  }
}
  1. Print a common string in react-dom warning messages, so we can filter them using Chrome devtools.
NullEnt1ty commented 5 months ago

Also is a PR welcome to improve this situation? As a first step, I'd love to have a chat with any maintainer here and figure out what acceptable solutions to this would be accepted. I don't know how to summon a maintainer though!

Looking at the history of react-devtools-extensions, it looks like @hoxyq might be the right one to summon. Could you help us out? 😇

hoxyq commented 5 months ago

Looking at the description, this issue is more than just changing how React DevTools' appends the component stack.

Regarding the long component stacks, I agree that there should be a better way of doing this, console.groupCollapsed seems like a nice solution, PRs / proposals are welcomed.

This is where RDT patches console - https://github.com/facebook/react/blob/96c58466109c2944adb817001fec38088a7c431e/packages/react-devtools-shared/src/backend/console.js#L198-L279

dbalatero commented 5 months ago

And just to clarify, I think the biggest pain point here for me is that:

On Fri, Apr 12, 2024, at 10:39 AM, Ruslan Lesiutin wrote:

Looking at the description, this issue is more than just changing how React DevTools' appends the component stack.

Regarding the long component stacks, I agree that there should be a better way of doing this, console.groupCollapsed seems like a nice solution, PRs / proposals are welcomed.

— Reply to this email directly, view it on GitHub https://github.com/facebook/react/issues/25447#issuecomment-2051888715, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAOQJKUUBOEUAGD2WY6TR3Y47WZLAVCNFSM6AAAAAAQ74U3ZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJRHA4DQNZRGU. You are receiving this because you commented.Message ID: @.***>

YannickBochatay commented 1 month ago

Still no news on this subject ? 😥