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

Fix issue overlapping argument with default value and alias with default value #734

Merged
merged 8 commits into from
Feb 8, 2023

Conversation

mununki
Copy link
Member

@mununki mununki commented Feb 7, 2023

@@ -4,11 +4,12 @@ module C0 = {
type props<'priority, 'text> = {priority: 'priority, text?: 'text}

@react.component
let make = ({priority: _, ?text, _}: props<'priority, 'text>) => {
let text = switch text {
let make = (props: props<'priority, 'text>) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Using props instead of destructuring to avoid the conflict from overlapped argument name with default value.

@mununki
Copy link
Member Author

mununki commented Feb 7, 2023

74e71b1 fixes the issue rescript-lang/rescript#5979
These two issues are tightly related, and I think it is better to fix them with this change.

@cristianoc
Copy link
Contributor

Could you also add tests that do compile, so it's clear what little mistakes are in there.
As in jscomp/test/recursive_react_component.res.

@mununki mununki changed the title Fix issue overlapping argument with default value Fix issue overlapping argument with default value and alias with defaul value Feb 7, 2023
@mununki mununki changed the title Fix issue overlapping argument with default value and alias with defaul value Fix issue overlapping argument with default value and alias with default value Feb 7, 2023
@mununki
Copy link
Member Author

mununki commented Feb 7, 2023

Could you also add tests that do compile, so it's clear what little mistakes are in there. As in jscomp/test/recursive_react_component.res.

Do you mean in the compiler repo?

@cristianoc
Copy link
Contributor

Could you also add tests that do compile, so it's clear what little mistakes are in there. As in jscomp/test/recursive_react_component.res.

Do you mean in the compiler repo?

Yes sorry, I mean, do this PR in the compiler repo first.

@mununki
Copy link
Member Author

mununki commented Feb 8, 2023

Backporting rescript-lang/rescript#5989

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.

Looking good.

@cristianoc cristianoc merged commit 2fd8f0c into master Feb 8, 2023
@cristianoc cristianoc deleted the fix-default-value-props-v4 branch February 8, 2023 16:14
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.

JSXv4: Unable to use argument with default value in other default arguments
2 participants