Implement more PropTypes #315
Conversation
This proptype translates to the same thing as the "function" and "Function" types, that is, any function. I extracted a helper to create this type, as it is used in two places.
This proptype works exactly like the "Object" type annotation, accepting any object.
samwgoldman
changed the title
When props flow into react element proptypes, they are not treated as object literals, because their reason is not exactly "object literal." This causes the props to be unified with the proptypes instead of compared using subtyping rules. The real fix to this is to carry around "this is literal" information separately from the reason string, but until we have that, I'm making this hack even dirtier. This has a nice benefit of simplifying type failures, as you can see in the changed test output. I verified that all removed error messages are inverses of messages that still exist -- i.e., we have not lost any type checking.
samwgoldman
changed the title
| let lit = (desc_of_reason reason1) = "object literal" in | ||
| let lit = | ||
| let desc = (desc_of_reason reason1) in | ||
| let is_prefix a b = |
There was a problem hiding this comment.
Here's a simpler is_prefix:
let is_prefix a b = String.(
let n = length b in
length a >= n && sub a 0 n = b)
There was a problem hiding this comment.
👍 I'll use that instead.
|
Awesome... hopefully the march of progress will sweep away the reason hacking in short order. :) Meanwhile, if you could substitute the simpler is_prefix (or an equivalent) I'll merge this baby in. Thanks again! |
|
Should be good to go. Only a few simple proptypes left. |
| @@ -47,10 +47,6 @@ new_react.js:4:12,33: string | |||
| This type is incompatible with | |||
| new_react.js:18:15,20: number | |||
|
|
|||
| new_react.js:4:12,33: string | |||
There was a problem hiding this comment.
Any idea why this goes away?
There was a problem hiding this comment.
Yeah, I mentioned this in the commit message for this change. Since we used to unify props, proptypes were checked in both directions. Elsewhere in this expectation the same error exists with the line numbers reversed.
|
Hi Sam - everything we talked about looks great. The error that gets dropped is a puzzle though... apologies for not noting it earlier. Any idea what's going on there? |
|
@bhosmer, yup, it's invariance vs subtyping. The error message used to fail "both ways" and now just fails one way. Intermediate states of this PR (before the proptype literal reason hacks) had a lot more of those "duplicated" errors. I think it's nicer now, and shouldn't entail a loss of type checking. |
|
Ah, cool - missed the inverted one later in the error dump. Thanks! |
Implemented
func,object, andobjectOf. Added tests for the existingarrayOfproptype. "Fixed" issue flowing React props to generated proptypes (see ba95d17).