Skip to content
This repository was archived by the owner on Jun 15, 2023. It is now read-only.

Remove key prop from JSX props type #635

Merged
merged 8 commits into from
Sep 17, 2022
Merged

Remove key prop from JSX props type #635

merged 8 commits into from
Sep 17, 2022

Conversation

mununki
Copy link
Member

@mununki mununki commented Sep 16, 2022

  • Remove the key prop from the props type
  • Add magic React.addKeyPropJsx.addKeyProp for the type checking

@mununki
Copy link
Member Author

mununki commented Sep 16, 2022

I noticed that type domProps has the key: string too. It is used for the lowercase such as <div key="k" />. Is it also needed to be removed? https://github.com/rescript-lang/rescript-react/blob/46944b490ee1923cfb69fa5ef0bd3095f93d18de/src/ReactDOM.res#L58

@mununki
Copy link
Member Author

mununki commented Sep 16, 2022

Does the lowercase have the same context? rescript-lang/rescript#5646 (comment)

@mununki
Copy link
Member Author

mununki commented Sep 16, 2022

I think the lowercase wouldn't be a problem, because it is still use the @deriving(abstract).

ReactDOMRe.createDOMElementVariadic("div", ~props=ReactDOMRe.domProps(~key="k", ()), []),

@cristianoc
Copy link
Contributor

I think lowercase is OK

@cristianoc
Copy link
Contributor

Would you update the spec too?

@mununki
Copy link
Member Author

mununki commented Sep 17, 2022

Would you update the spec too?

0fbf9a7

Copy link
Contributor

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great.
Anything missing?

@mununki
Copy link
Member Author

mununki commented Sep 17, 2022

This is great. Anything missing?

Thanks.
No, it seems done.

@cristianoc cristianoc merged commit b911f84 into master Sep 17, 2022
@mununki mununki deleted the remove-key-prop branch September 17, 2022 13:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants