-
Notifications
You must be signed in to change notification settings - Fork 469
Extend genType to recognise JSX V4 components. #5619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
No special treatment so far, just a normal function as if it were written by hand.
export const CompV3: { make: React.ComponentType<{ readonly x: string; readonly y: string }> } = JSXV4BS.CompV3 | ||
|
||
export const CompV4: { make: React.ComponentType<{ | ||
readonly key?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattdamon108 why does the type of props
include key
?
Normally one never calls the function directly passing key
, but uses instead the same mechanism as for refs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless the type props
has the key, the type checking would not pass for the caller site.
<Comp key="key" />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSX apply can treat key specially and simply add an explicit key:string type check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me take a look at the spec and make a concrete suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current:
<Comp x key="7">
// is converted to
React.createElement(Comp.make, {x, key: "7"})
New:
<Comp x key="7">
// is converted to
React.createElement(Comp.make, {x, key: ("7":string)})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current:
<Comp x key="7"> // is converted to React.createElement(Comp.make, {x, key: "7"})New:
<Comp x key="7"> // is converted to React.createElement(Comp.make, {x, key: ("7":string)})
Not sure I understand your intention.
Doesn't the New
still needs the type props including key
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current:
<Comp x key="7"> // is converted to React.createElement(Comp.make, {x, key: "7"})New:
<Comp x key="7"> // is converted to React.createElement(Comp.make, {x, key: ("7":string)})Not sure I understand your intention. Doesn't the
New
still needs the type props includingkey
?
I think that's something one can work around.
But the lack of an empty record, one cannot work around I think #5623
So for now I'll filter it out on the genType side, even if it's a bit of magic that ideally I would prefer not to put in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filtering out done here bfe431c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side comment: I think you already know. The type props including key
is needed, because the React.component has a type definition like this. https://github.com/rescript-lang/rescript-react/blob/master/src/React.res#L11:L19
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side comment: I think you already know. The type props including
key
is needed, because the React.component has a type definition like this. https://github.com/rescript-lang/rescript-react/blob/master/src/React.res#L11:L19
This is what I think one could work around, but the empty record, I don't think one can.
No special treatment so far, just a normal function as if it were written by hand.