Fix #6153: Warn for PropTypes that don't exist but do exist with a di…

[ghost]

No description provided.

[jimfb]

The indentation here is not consistent with the rest of our code.

[jimfb]

Overall, this looks good. Just a few nitpicks.

Also, can you add a full-path test that actually creates a component with a proptype specified, and then calls React.render with a miss-capitalized proptype? We want to make sure the public API semantics are tested.

[ghost]

@jimfb I implemented and ran the test you described and here's what happened: the warning was output twice. Once with React.createElement and then again with ReactDOM.render. However, if the chainable type checker returned a new Error as opposed to using the devtool to output a warning, it is output only once. How do you suggest I proceed?

[jimfb]

That would imply that createChainableTypeChecker is being called twice (once at createElement and once at ReactDOM.render). I didn't sit down and think about why that might be happening, it surprises me a little - it might be worth looking into.

We are probably going to need to add some deduplication logic into your devtool anyway. We don't the warning to fire every single time the component is rerendered. You can take a look at the other places we emit warnings (eg. DOMPropertyOperations.js contains illegalAttributeNameCache and validatedAttributeNameCache), and you can do something similar within your devtool.

[facebook-github-bot]

@ahmedghoneim92 updated the pull request.

[ghost]

@jimfb I found the two places from which it gets called. Once from ReactElementValidator.checkPropTypes at element creation and again from ReactCompositeComponentMixin._checkPropTypes at rendering. There's actually a "TODO: Stop validating prop types here and only use the element validation" written in the second function.

I've updated my code with a cache of miscapitalized propNames, is this okay, or should I cache the (componentName, propName) combination?

[gaearon]

Please don’t use for of syntax as it relies on ES6 Symbol being available, and we don’t polyfill it.

[gaearon]

As I wrote in #6255 (comment), onCreateChainableTypeChecker seems like a misnomer because the function actually runs during the type check—not during creating a type checking function. The indentation might be a little confusing there, but you’ll notice the call is inside checkType.

In general, I’m not sure if firing events about propTypes being checked is something we want to do. Since we plan to eventually remove them from the core, we don’t want to couple them to ReactInstrumentation. In other words, firing debug tool events from inside prop type validation code feels wrong to me because it means prop type validation becomes more tied to React.

Also, is this a breaking change? I can imagine some components in the ecosystem might have incorrect propTypes which might suddenly start emitting warnings. Some of those components might not even be maintained anymore, and there is nothing a user can do to opt out of those warnings. If some component’s propTypes casing does not match the actual casing of props it receives, we might even encourage the user to introduce a bug by adding this feature.

Overall I see how this might be useful but I’m not sure if checking casing is how I’d approach this. This is a subset of a more general problem: warning for extraneous prop types. We discussed this in #1587 but I don’t think it went anywhere because of the pattern of transferring props.

I’m probably not the right person to review this. @spicyj and @sebmarkbage might have more context on this. So I’m just commenting to explain why we might not be ready to proceed with this.

[sebmarkbage]

some components in the ecosystem might have incorrect propTypes which might suddenly start emitting warnings

That's going to be an issue with every warning we add. Yet, we need to add warnings. Plausible solutions include blackboxing warnings or version flags on the warning modules so you can opt-in to only new warnings. Orthogonal to this PR, but might affect which version it gets merged into.

This is a subset of a more general problem: warning for extraneous prop types

That is true but since that's a much more substantial change, we might as well move quickly on this one. Even with the other warning, checking for casing is a better error message than just "typo". You can give the user a concrete suggestion for the fix.

I share the concern about exposing this contract in the devtool though. As a meta-point I'm skeptical of the architecture of building warnings in the devtool. I think it's over architected. I think it makes a lot more sense to just expose a "warning" hook in the devtool that lets it do what it wants with it, but not building the warnings themselves in the devtool. It exposes too many fast moving implementation details.

[gaearon]

I think it makes a lot more sense to just expose a "warning" hook in the devtool that lets it do what it wants with it, but not building the warnings themselves in the devtool.

As in, core emits the warnings, devtools decide whether to log / present / record them?

[omeid]

some components in the ecosystem might have incorrect propTypes which might suddenly start emitting warnings

If you have incorrect proptypes, the component is broken. What is the point of avoiding errors for something that is most likely broken than not?

[mangafas]

why this isn't merged?

[aweary]

The original author of this PR has deleted their Github account. Since there are alternative PRs already open for this (#6938, #7697) I'm going to close this one out.