Skip to content
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

[RFC] Refine logging strategy #21979

Open
oliviertassinari opened this issue Jul 28, 2020 · 2 comments
Open

[RFC] Refine logging strategy #21979

oliviertassinari opened this issue Jul 28, 2020 · 2 comments
Labels
discussion ready to take Help wanted. Guidance available. There is a high chance the change will be accepted RFC Request For Comments

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 28, 2020

Context

This is a follow-up on #15343. Looking at the components (under /packages/*), we can count the following usage frequency of the different logging methods:

  • console.error x69
  • console.warn x25

Historically, React used to use console.warn. It was changed in facebook/react#3440 to benefit from the stack trace of error. Something Chrome now supports for warn now:

Capture d’écran 2020-07-28 à 13 29 31

Problem

It doesn't seem that there is any rule or convention around when to use error over warn. Do we have an improvement opportunity here? Could we use it as leverage to help developers that need to prioritize which problem they should focus on first?

Proposal

What do you think of:

  • If we are unsure, better say it's an error over warn to be safe.
  • console.error we use it for cases that are important. It can be a crash of the page, explaining what went wrong, or creating awareness about a significantly wrong user-experience. For instance:
    console.error(
    [
    'Material-UI: There are multiple InputBase components inside a FormControl.',
    'This is not supported. It might cause infinite rendering loops.',
    'Only use one InputBase.',
    ].join('\n'),
  • console.warn is used as soon as something is not right, leading to incorrect behavior but not critical. For instance, these should be warnings:
    console.error(
    'Material-UI: <StepContent /> is only designed for use with the vertical stepper.',
    );

    console.warn(
    [
    'Material-UI: Motorcycle icon has been replaced with TwoWheeler.',
    "Please change your import to `import Motorcycle from '@material-ui/icons/TwoWheeler';`",
    ].join(' '),

    console.error(
    [
    'Material-UI: You have provided an invalid combination of props to the Breadcrumbs.',
    `itemsAfterCollapse={${itemsAfterCollapse}} + itemsBeforeCollapse={${itemsBeforeCollapse}} >= maxItems={${maxItems}}`,
    ].join('\n'),
    );

    console.error(
    [
    "Material-UI: The GridList component doesn't accept a Fragment as a child.",
    'Consider providing an array instead.',
    ].join('\n'),
@oliviertassinari oliviertassinari added discussion ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Jul 28, 2020
@vicasas
Copy link
Member

vicasas commented Apr 4, 2021

I really like this approach, I feel that it is something necessary to implement in all places where possible. Giving feedback to the developer as soon as possible on problems using the components is great.

Treat everything that bursts with error and treat what is not right using warn

@shelbyspeegle
Copy link

shelbyspeegle commented Apr 14, 2021

I really like this as well.

In particular, the one that is driving us insane is Material-UI: The contrast ratio of... errors. Maybe we are doing something wrong but seeing this three line error six times on load is something we have gotten used to over the last three or so years, making it harder to see errors that actually need our attention. Being able to filter console output to errors only is something we do frequently as we have a similar error/warn prioritization going on in our own code that follows the guidelines that you describe.

I know there has been an ongoing debate on whether or not there should be a configuration or setting to remove the error logging for contrast ratio. I feel like this error/warn prioritization would ease the pain a little bit if we could change that message to a console.warn instead of console.error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion ready to take Help wanted. Guidance available. There is a high chance the change will be accepted RFC Request For Comments
Projects
None yet
Development

No branches or pull requests

3 participants