Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix issue with JSX V4 when components are nested #6031

Merged
merged 1 commit into from
Mar 5, 2023
Merged

Fix issue with JSX V4 when components are nested #6031

merged 1 commit into from
Mar 5, 2023

Conversation

cristianoc
Copy link
Collaborator

@cristianoc cristianoc commented Mar 3, 2023 •
edited
Loading

Fixes #5975

The issue is due to a combination of:

  1. when a component let make = body is transformed, first the body is transformed, then when transformStructureItem4 is called, the body is transformed again
  2. @react.component is left after the PPX transformation

Because of this, in the case of nested components, the transformed inner components is transformed again by transformStructureItem4, but it should not as it's been desugared already.

It would be enough to change either one, to avoid the error with nested components, but this PR handles both aspects.

@cristianoc cristianoc requested a review from mununki March 3, 2023 21:19
@cristianoc
Fix issue with JSX V4 when components are nested
eb623b7 
Fixes #5975

The issue is due to a combination of:
1) when a component let make = body is transformed, first the body is transformed, then when transformStructureItem4 is called, the body is transformed again
2) @react.component is left after the PPX transformation

Because of this, in the case of nested components, the transformed inner components is transformed again by transformStructureItem4, but it should not as it's been desugared already.

It would be enough to change either one, to avoid the error with nested components, but this PR handles both aspects.
@mununki
Copy link
Member

mununki commented Mar 4, 2023 •
edited
Loading

Isn't it still valid to be a transformation from inside? I'm not sure why we're deleting the mapper, it seems like we only need to remove the @react.component attribute at the end of the inner transformation.

cristianoc
cristianoc commented Mar 4, 2023
View reviewed changes
res_syntax/src/reactjs_jsx_ppx.ml
@@ -127,8 +127,8 @@ let getMapper ~config =
| Pstr_attribute attr -> processConfigAttribute attr config
| _ -> ());
let item = default_mapper.structure_item mapper item in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This applies the JSX transformation recursively to the structure item.

cristianoc
cristianoc commented Mar 4, 2023
View reviewed changes
res_syntax/src/reactjs_jsx_ppx.ml
@@ -127,8 +127,8 @@ let getMapper ~config =
| Pstr_attribute attr -> processConfigAttribute attr config
| _ -> ());
let item = default_mapper.structure_item mapper item in
if config.version = 3 then transformStructureItem3 mapper item
else if config.version = 4 then transformStructureItem4 mapper item
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was applying the JSX transformation recursively again, to some parts of the structure item.

@cristianoc
Copy link
Collaborator Author

Isn't it still valid to be a transformation from inside? I'm not sure why we're deleting the mapper, it seems like we only need to remove the @react.component attribute at the end of the inner transformation.

See the comment here: https://github.com/rescript-lang/rescript-compiler/pull/6031/files#r1125400463
The question is why is there was a mapper there in the first place.

cristianoc
cristianoc commented Mar 4, 2023
View reviewed changes
res_syntax/src/reactjs_jsx_v3.ml
@@ -446,8 +446,7 @@ let jsxMapper ~config =
args
in

let rec recursivelyTransformNamedArgsForMake mapper expr args newtypes =
let expr = mapper.expr mapper expr in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would apply the entire JSX transform again, to expr, every time this function is entered.
I guess at best this would do nothing as the transform has happened already.
If it does something, it's probably not going to be something good.
Am I missing something @mununki ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why I didn't realize it didn't need this.

cristianoc added a commit to rescript-lang/syntax that referenced this pull request Mar 4, 2023
@cristianoc
Port of PR Fix issue with JSX V4 when components are nested rescript-…
d491e13 
…lang/rescript#6031
@cristianoc cristianoc mentioned this pull request Mar 4, 2023
Merged
cristianoc added a commit to rescript-lang/syntax that referenced this pull request Mar 4, 2023
@cristianoc
Port of PR Fix issue with JSX V4 when components are nested rescript-…
3333ffe 
…lang/rescript#6031
@cristianoc
Copy link
Collaborator Author

@mununki any last thoughts on this? Going to merge this soon, as it's the last outstanding fix remaining for release 10.1.3.

@mununki
Copy link
Member

mununki commented Mar 5, 2023

Sorry for coming back so late, I didn't want to be a blocker.
I just thought it was weird that the mapper function didn't fit my mental model of having a mapper as an argument, so I sat down at my computer and did some debugging.
I think this PR is right, the function that transforms the structure_item, signature_item doesn't need a mapper, since defalut_mapper.structure_item would have already transformed the expr recursively as well.

cristianoc added a commit to rescript-lang/syntax that referenced this pull request Mar 5, 2023
@cristianoc
Port of PR Fix issue with JSX V4 when components are nested rescript-…
396df51 
…lang/rescript#6031
@cristianoc
Copy link
Collaborator Author

Sorry for coming back so late, I didn't want to be a blocker.
I just thought it was weird that the mapper function didn't fit my mental model of having a mapper as an argument, so I sat down at my computer and did some debugging.
I think this PR is right, the function that transforms the structure_item, signature_item doesn't need a mapper, since defalut_mapper.structure_item would have already transformed the expr recursively as well.

Later on one can clean up the code, and move the line let item = default_mapper.structure_item mapper item in inside the transform function.

@cristianoc cristianoc merged commit 168445a into master Mar 5, 2023
@cristianoc cristianoc deleted the jsx_v4_nested branch March 5, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mununki mununki mununki approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JSXv4: Nested component definitions yields confusing error
2 participants
@cristianoc @mununki