rescript-lang / rescript-compiler

The compiler for ReScript.
https://rescript-lang.org
Other
6.74k stars 448 forks source link

React/JSX broken after external arity handling improvement #6880

Closed cknitt closed 3 months ago

cknitt commented 3 months ago

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 commented 3 months ago

https://github.com/rescript-lang/rescript-compiler/pull/6881

cknitt commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 https://github.com/rescript-lang/rescript-compiler/issues/5725

cknitt commented 3 months ago

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 commented 3 months ago

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: https://github.com/rescript-lang/rescript-compiler/pull/6883

Can you try the full project?

cknitt commented 3 months ago

Can you try the full project?

Works fine now 🎉! Thanks a lot!