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.

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

Special JSX children error message #7044

Merged
merged 2 commits into from
Sep 24, 2024
Merged

Conversation

zth
Copy link
Collaborator

@zth zth commented Sep 21, 2024

This special cases the error message you get when you pass children to a JSX component that does not accept them. The big reason for special casing just this prop is that passing it is already special cased in the syntax (<div>{React.string("this is children")}</div>) vs passing a regular prop ( <div className="this is a regular prop" />).

I think this makes it clearer, but I'd love to hear some feedback.

Part of #6975

@res.jsxComponentProps
type props<'name> = {name: 'name}

let make = (): props<'name> => {children: ""}
Copy link
Member

Choose a reason for hiding this comment

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

I don't get this. Is this really what the transform would output?

Why is the make function returning props? And not React.element?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No it's not what the transform would output, but it doesn't matter, what matters is that a record of the given type is being created, which is what the transform would do. If we want to make this clearer we could set up a test suite with the transform enabled. But I don't know whether that's cumbersome to do. Do you know?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cknitt the problem now is that React itself isn't included. There's a react_ppx folder that does similar tests with the transform and React copied into the project itself. We could mimic that, but that's a larger change.

Copy link
Member

Choose a reason for hiding this comment

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

@zth I don't think you need React, you can use the Jsx module that comes with the compiler.

E.g. something like

@jsx.component
let make = (~name) => Jsx.string(name)

should work fine.

Copy link
Member

@cknitt cknitt Sep 23, 2024

Choose a reason for hiding this comment

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

Ok, well, you don't need it in my example, but you are right, you need the externals for the JSX runtime if you want to use JSX.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cknitt let's just worry about the actual error message in this PR, and then we can figure out how to set up the React transform for this separately.

@zth zth merged commit e6fc84e into master Sep 24, 2024
20 checks passed
@zth zth deleted the jsx-no-children-prop-error-message branch September 24, 2024 06:45
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.

3 participants