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

React/JSX broken after external arity handling improvement #6880

Closed
cknitt opened this issue Jul 15, 2024 · 8 comments
Closed

React/JSX broken after external arity handling improvement #6880

cknitt opened this issue Jul 15, 2024 · 8 comments

Comments

@cknitt
Copy link
Member

cknitt commented Jul 15, 2024

After #6874, I have the following problem:

module FormattedMessage = {
  @react.component @module("react-intl")
  external make: (~id: string, ~defaultMessage: string) => React.element = "FormattedMessage"
}

let _ = <FormattedMessage id="test" defaultMessage="Test" />

compiles to

JsxRuntime.jsx((function (prim) {
  return ReactIntl.FormattedMessage;
}), {
  id: "test",
  defaultMessage: "Test"
});

(which fails at runtime) instead of

JsxRuntime.jsx(ReactIntl.FormattedMessage, {
      id: "test",
      defaultMessage: "Test"
    });

I think the root cause is that React.component is not defined as an abstract type, but as 'props => React.element. @mununki tried to make it abstract in #6304, but hit a roadblock.

Although I am also asking myself why the compiler needs to introduce that extra function now that it knows that the external is a function of arity 1.

cristianoc added a commit that referenced this issue Jul 16, 2024
@cristianoc
Copy link
Collaborator

#6881

cristianoc added a commit that referenced this issue Jul 16, 2024
cristianoc added a commit that referenced this issue Jul 16, 2024
@cknitt
Copy link
Member Author

cknitt commented Jul 16, 2024

@cristianoc I retested with latest master and am now getting

JsxRuntime.jsx((function (prim) {
  return ReactIntl.FormattedMessage(prim);
}), {
  id: "test",
  defaultMessage: "Test"
});

for the above example.

This will still fail at runtime.

@cknitt cknitt reopened this Jul 16, 2024
@cknitt
Copy link
Member Author

cknitt commented Jul 16, 2024

It does look correct in the test in your PR, however.
But there you preceded it with

module React = {
  type element = Jsx.element
  type componentLike<'props, 'return> = 'props => 'return
  type component<'props> = Jsx.component<'props>

  @module("react/jsx-runtime")
  external jsx: (component<'props>, 'props) => element = "jsx"
}

If I do that, it works for me, too.
But not when just using the normal @rescript/react bindings in my project.

@cristianoc
Copy link
Collaborator

It does look correct in the test in your PR, however. But there you preceded it with

module React = {
  type element = Jsx.element
  type componentLike<'props, 'return> = 'props => 'return
  type component<'props> = Jsx.component<'props>

  @module("react/jsx-runtime")
  external jsx: (component<'props>, 'props) => element = "jsx"
}

If I do that, it works for me, too. But not when just using the normal @rescript/react bindings in my project.

Can you give me a super-quick repro in its own repo?
Maybe an empty react project that only has my example in it, or something.

@mununki
Copy link
Member

mununki commented Jul 16, 2024

Side note: I think the problem with React.component not being an abstract type is when the user defines the component as a function signature in a case where @react.component is not used. related to #5725

@cknitt
Copy link
Member Author

cknitt commented Jul 16, 2024

Can you give me a super-quick repro in its own repo?

@cristianoc https://github.com/cknitt/rescript-repro-6880

(It uses the npm package downloaded from CI here, you will have to adapt that accordingly:
https://github.com/cknitt/rescript-repro-6880/blob/66ccf9e5667f72eefde07255f2d4491af74c582b/package.json#L16)

@cristianoc
Copy link
Collaborator

Can you give me a super-quick repro in its own repo?

@cristianoc https://github.com/cknitt/rescript-repro-6880

(It uses the npm package downloaded from CI here, you will have to adapt that accordingly: https://github.com/cknitt/rescript-repro-6880/blob/66ccf9e5667f72eefde07255f2d4491af74c582b/package.json#L16)

Great. I had a PR ready to go that simply implements things in a more robust way, and it seems to work:
#6883

Can you try the full project?

@cknitt
Copy link
Member Author

cknitt commented Jul 16, 2024

Can you try the full project?

Works fine now 🎉! Thanks a lot!

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

No branches or pull requests

3 participants