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

Improve the warning strategy #15343

Closed
eps1lon opened this issue Apr 14, 2019 · 10 comments
Closed

Improve the warning strategy #15343

eps1lon opened this issue Apr 14, 2019 · 10 comments
Labels
core Infrastructure work going on behind the scenes
Milestone

Comments

@eps1lon
Copy link
Member

eps1lon commented Apr 14, 2019

How do you guys want to handle the warning thing in the future, there is a wide range of different options. There are two problems with a strategy like this:

https://github.com/mui-org/material-ui/blob/8ae684d7c9d884c84d89c40da88b46254e5d53a2/packages/material-ui/src/RadioGroup/RadioGroup.js#L39-L52

  • We have extra code that is not removed in production.
  • We raise more warnings in the console than necessary. It's creating noise.

Maybe?

import React from 'react';

/* to package into it's own module */
const dedup = {};
function warning(message, type) {
  if (!dedup[type]) {
    React.warn(messasge);
    dedup[type] = 1;
  }
}

function MyComponent() {
  if (foo === bar) {
    warning('No, you should not bazzz', 'MyComponent');.
  }
}

plus https://github.com/oliviertassinari/babel-plugin-transform-dev-warning

Another option:

import React from 'react';

/* to package into it's own module */
const dedup = new WeakMap();
function warning(condition, message) {
  const { current } = React.useRef({});
  if (!condition && !dedup.has(current)) {
    React.warn(messasge);
    dedup.set(current);
  }
}

function MyComponent() {
  warning(foo !== bar, 'No, you should not bazzz');.
}

Originally posted by @oliviertassinari in #15291 (comment)

@eps1lon eps1lon added the core label Apr 14, 2019
@oliviertassinari oliviertassinari self-assigned this Apr 14, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 18, 2019

@oliviertassinari oliviertassinari added this to the v5 milestone Apr 19, 2019
@oliviertassinari oliviertassinari removed their assignment Apr 19, 2019
@oliviertassinari oliviertassinari changed the title Cache warnings per component Improve the warning strategy Apr 21, 2019
@oliviertassinari
Copy link
Member

React has reverted the warn and error helpers: facebook/react#16126.

@eps1lon
Copy link
Member Author

eps1lon commented Sep 11, 2019

I'm going to evaluate our custom propTypes and see what we can move into custom console.warn and console.error. The new react devtools should append the component stack to those now.

@oliviertassinari
Copy link
Member

Sweet, from what @merceyz said, it could unlock the migration of more components to the props description in the TypeScript defintions :).

@eps1lon
Copy link
Member Author

eps1lon commented Sep 19, 2019

Referencing #17480 here. It's probably good to have some link/section about debugging react apps. The error overlay for create-react-app prioritizes the JS stack trace and has a barely legibly notice about console. We need to make sure devs are aware that they need to read (and address) the console output top-down not bottom-up .

Izhaki added a commit to Izhaki/regraph that referenced this issue Sep 29, 2019
@oliviertassinari
Copy link
Member

@eps1lon Since #17404 was merged, what do we miss to close the issue?

@eps1lon
Copy link
Member Author

eps1lon commented Oct 6, 2019

@eps1lon Since #17404 was merged, what do we miss to close the issue?

Go through current console.error and decide if console.warn would be sufficient.

@oliviertassinari
Copy link
Member

Looking at the codebase, I can count:

  • console.error x68
  • console.warn x7

What do you think of reserving the usage of console.error to the cases where the page actually crashes? console.warn is used as soon as something is not right, leading to incorrect behavior.

@eps1lon
Copy link
Member Author

eps1lon commented Jul 22, 2020

Closing this for now unless React.warn gets re-introduced.

@eps1lon eps1lon closed this as completed Jul 22, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 22, 2020

Did you decide on a policy around console.warn vs console.error (when to use which one)? #15343 (comment)

@oliviertassinari oliviertassinari added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 20, 2022
@zannager zannager added core Infrastructure work going on behind the scenes and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

No branches or pull requests

3 participants