React: Warning message is swallowed when using shape inside oneOfType

[jochenberger]

See http://codepen.io/jochenberger/pen/egVNzX
I'd expect a warning that says "Invalid prop bar supplied to App. A foo should not have a bar.' Or even better "Invalid prop foo.bar supplied to...".
But I only get "Invalid prop foo supplied to App".

[niole]

I second "Invalid prop foo.bar"

[niole]

I'm not sure if this is really an issue @jochenberger. If you have a union type and none of the types pass, all that matters is that your union type failed. If you have a single nested shape prop which isn't sitting in a union type, you should get your custom error.

[jochenberger]

Yes, I noticed that too. I thought that since React already knows that foo is an object, It could provide me with the detailed message.
There's however a problem if I do something like this (yes, I know, you wouldn't do it this way):

foo: React.PropTypes.oneOfType([
       React.PropTypes.shape({
        bar: PropTypes.number.isRequired
      }),
       React.PropTypes.shape({
        bar: PropTypes.string.isRequired
      })
    ])

and I pass foo={ { bar: [ ] } }.
So the problem is with union types rather than with shape.
I'm not sure how this could be improved. Technically, the foo is broken here, but somehow the error message seems wrong.

[zacbraddy]

I think I can see both sides of the argument here. Yes, foo doesn't match the propType criteria and because one of those predicates has failed the union type has failed and to some degree we don't care why it has failed we just know it has. However, I can also see the other side in that just because we have wrapped the shape in an union type shouldn't change the behaviour of it.

I think I tend to agree with the latter here. I think the shape should give the same expected behaviour whether or not it is in a union type.

I'm willing to have a stab at this if that's alright with everyone?

[zacbraddy]

Ok having dug into this more I now have a better understanding of what @niole was saying and it seems that I completely missed the point she was making :laughing: I now get it.

Let's take the example where you have 10 checks in your oneOfType array. The positive way of looking at this is that you are wanting your prop to match at least one of these criteria. We can have a more interesting discussion though if we talk about the negative point of view here. What state should we have when this check fails? When this fails we expect that all the checks were iterated over and all 10 of your checks have failed.

Currently react just reports to you which oneOfType validator failed. This, in theory gives, you a good place to go look for why that propType validation failed. Assuming that we don't like this and we want more information how could we technically facilitate delivering this information?

The way I see it you've got two courses of action. Either report the way in which all 10 failed or report the way in which just one failed (i.e. the first or the last failure). Neither of these feels right to me.

Showing all 10 failures doesn't seem useful because you only have to match one of them, not all 10! Plus what if you had more than 10? What if you had 100? Do we report them all now? 100 checks is an unlikely scenario, yes, but not physically impossible to do either.

Equally showing just one doesn't seem right either because it failed all the other tests, right? Why are we only reporting one? What if the developer was meaning to meet one of the other propType validators and we've only reported the first one? That would be a confusing turn of events I think.

I believe that given all I've just said the current functionality might be the right one in this instance. React should report the prop that is using the oneOfType check has failed it's validation, as it does. From there React can dust it hands off and feel good about having pointed the developer in the right direction to fix their own problem.

[jochenberger]

I can live with that. Not showing the details just feels strange in my example (where I say that foo is to be either a string or an object with a certain shape).
E.g.:

App.propTypes  = {
  foo: React.PropTypes.oneOfType([
       React.PropTypes.string, 
       React.PropTypes.shape({
        bar: React.PropTypes.number
      })
    ])
}
ReactDOM.render(
  <App foo={{bar: "x"}}/>,
  document.getElementById('app')
);

React telling me that my foo is not valid just feels wrong.
The oneOfTypecheck could probably rule out the options that have a wrong type altogether (I'm not passing a string as foo, React knows that I'm passing an object and only the shape option can match that), and, if there is only a single option left, include its error message in the overall error, but maybe that's not worth the effort. It's a trade-off between providing as much information as possible in the error report and making things overly complicated.
And after all, it's not a user-facing error but DEV mode warning and the developer will probably be able to figure out what causes it.

[brion-fuller]

Would it be better to reword the error message that is returned? So instead of stating, "Invalid prop foo supplied to App" could we instead state something like "No match found for prop foo supplied to App". This would make it a little more clear to the error message.

[arshabh]

I concur this is a non issue and also think that this would open pandora's box if we go after Shape. Because then we should also handle similar situations in arrayOf, objectOf etc. Also i agree with comments above that a pointer in the right direction is enough and extra details are not worth the effort.

[punya]

When checking a value against a possible type, we could define a mismatch score based on the following rules:

• If a type agrees with a value, we give it a score of 0
• If a type disagrees with a value at the top level (e.g. propTypes.string and {"foo": 10}), we give it a score of 1.0
• For a propTypes.shape type, we average the scores for the fields
• For a propTypes.union type, we take the minimum score among the cases
• For a propTypes.arrayOf or propTypes.objectOf type, we take the maximum score among the items

As we propagate scores, we could carry forward or manipulate the error message, so that in the end we output the error message that's likely to be most helpful to the user.

I should acknowledge that this is a pretty heavyweight approach, so it's worth thinking carefully about whether it's actually worth the effort.

[niole]

Honestly, in my experience as a software developer, if I proposed revisions for a message which already works, these revisions would need to go through extensive review. It's difficult to say whether a more detailed message adds anything at all. The smart thing to do is to leave it alone rather than sink a bunch of time into it.

[punya-asapp]

@niole, @arshabh: that's a reasonable argument. Perhaps the issue should be closed out in that case?

[zacbraddy]

I agree that the risk surface and effort required to achieve this result out weighs the reward too much to justify it going ahead. I also feel this can be closed.

[aweary]

I agree with all the points made above about this being a non-issue and will close this 😉