Skip to content
This repository has been archived by the owner on Sep 1, 2024. It is now read-only.

Output warnings using console.warn #245

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

guilhermearaujo
Copy link

Checking the prop types does not change how the program will actually work, and I believe that's why all invalid props are reported with a Warning message.

Yet, these messages are output using console.error, and this log level clearly conflicts with the message idea.

This PR replaces all those calls with console.warn, bringing consistency to the log levels.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@ljharb
Copy link
Contributor

ljharb commented Dec 20, 2018

It should be an error; if PropTypes are invalid, it means you have a bug.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@guilhermearaujo
Copy link
Author

Then why does the text say Warning? Should it be changed to Error?

@ljharb
Copy link
Contributor

ljharb commented Dec 20, 2018

It’s using console.error because it’s an error; i assume it’s saying “warning” because it doesn’t actually stop you from misusing your components.

@guilhermearaujo
Copy link
Author

Exactly, it still allow the usage of the components. I agree that an inconsistency in the prop types may end in a potential actual error. But it also may not.

JS is not strongly typed and things can work even when the developer is not very thoughtful about their code correctness (but let's not bring this to the discussion).

My argument is that the checking routine does not affect the code execution. The program will still run as it is intended (read: as the code says to run). If the validation should report an error, it would make more sense, in my opinion, to throw an actual error or exception, interrupting the execution.

If the intention is to "just let the developer know", a warning does look more appropriate, I believe.

@ljharb
Copy link
Contributor

ljharb commented Dec 20, 2018

It’s a guaranteed error if explicit PropTypes are invalid - either in the propTypes or in the consumer code. That it still might function correctly by accident doesn’t mean it’s not an error.

console.error is the right place to send this error output.

@eps1lon
Copy link

eps1lon commented Jan 13, 2019

It's not an either or answer. Sure if I expect an object in a prop and the user gives me a number then my component will not work as intended hence the error.

However what about e.g. deprecation warnings in prop types? You could argue that this can still be achieved by using console.warn. However propTypes validation has two very important features:

  1. caching
  2. component stack in the message

The first can be implemented in userland. The second not as far as I know. It seems like that it would be more appropriate to open a rfc on the react repo and discuss if propTypes should be able to control the level of logging.

@ljharb
Copy link
Contributor

ljharb commented Feb 21, 2019

Closing as a duplicate of #63.

@ljharb ljharb closed this Feb 21, 2019
@eps1lon
Copy link

eps1lon commented Feb 21, 2019

@ljharb That PR only downgrades the deprecation warning from console.error to console.warn. This PR downgrades all. If anything #63 should be closed over this one.

@ljharb
Copy link
Contributor

ljharb commented Feb 21, 2019

ah. in that case I'll reopen, since it's not a duplicate.

@Gsiete
Copy link

Gsiete commented Aug 3, 2019

In the meantime I'm using:

console.error = (function () {
  const { error } = console;

  return function (...args) {
    if (args[0] && args[0].match && args[0].match(/^warn(ing)?:/i)) {
      console.warn(...args);
    } else {
      error.apply(console, args);
    }
  };
}());

One particular case where I don't want to see it as an error is: reactstrap/reactstrap#1596

@asyncanup
Copy link

@facebook-github-bot @ljharb can we please get this merged? don't want to fork or work around the issue like @Gsiete did, unless absolutely needed.

@ljharb
Copy link
Contributor

ljharb commented Aug 27, 2019

I'm not convinced this is worth it; it'd be a breaking change, and could introduce a lot of code churn in test suites that check for emitted propType errors.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants