prop-types
prop-types copied to clipboard
facebook
Output warnings using console.warn
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.
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!
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
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.
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.
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.
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:
- caching
- 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 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.
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: https://github.com/reactstrap/reactstrap/issues/1596
@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.
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.