Skip to content

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

Merged
merged 14 commits into from
Aug 31, 2022
Merged

Conversation

cristianoc
Copy link
Collaborator

No special treatment so far, just a normal function as if it were written by hand.

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;
Copy link
Collaborator Author

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.

Copy link
Member

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" />

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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)})

Copy link
Member

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?

Copy link
Collaborator Author

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?

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.

Copy link
Collaborator Author

@cristianoc cristianoc Aug 31, 2022

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

Copy link
Member

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

Copy link
Collaborator Author

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.

cristianoc added a commit that referenced this pull request Aug 31, 2022
@cristianoc cristianoc changed the title Add example of JSX V4 with genType. Extend genType to recognise JSX V4 components. Aug 31, 2022
@cristianoc cristianoc merged commit bf2a8b7 into master Aug 31, 2022
@cristianoc cristianoc deleted the gentype_jsx_v4 branch August 31, 2022 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants