rescript-lang / rescript-compiler

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

External binding to component without `@react.component` #5725

Open mununki opened 1 year ago

mununki commented 1 year ago

https://forum.rescript-lang.org/t/call-for-help-2-test-jsx-v4/3781/22?u=moondaddi As of JSX v4, the component can be written without @react.component. The issue came from the binding to the component with external.

type props<'msg> = {msg: 'msg}
@module("@foo/bar")
external make: props<_> => React.element = "SomeComponent"
// output
React.createElement((function (prim) {
                          return Bar.SomeComponent(prim);
                        }), {
                        msg: "!!!"
                      })

// expected
React.createElement(Bar.SomeComponent(prim), {
                        msg: "!!!"
                      })

The output after transformation @react.component is different from the above.

type props<'msg> = {
  msg: 'msg,
}
@module("@foo/bar")
external make: React.componentLike<props<string>, React.element> = "SomeComponent"

The generated js output gets different between componentLike<'props, element> and props<_> => React.element.

mununki commented 1 year ago

I think this is not an issue. This is the matter of what you bind to. So, the proper binding is to the component not function.

// binding to the function
type props<'msg> = {msg: 'msg}
@module("@foo/bar")
external make: props<_> => React.element = "SomeComponent"

// binding to the component
type props<'msg> = {msg: 'msg}
@module("@foo/bar")
external make: React.componentLike<props<string>, React.element> = "SomeComponent"

What do you think @cristianoc @cknitt ?

mununki commented 1 year ago

Encouraged binding for external seems:

type props<'msg> = {msg: 'msg}
@module("@foo/bar")
external make: React.component<props<_>> = "SomeComponent"
cknitt commented 1 year ago

I do not understand the reason for the difference in the output. After all, React.component<'a> is the same as 'a => React.element.

React.component<'a>
= Jsx.component<'a>
= Jsx.componentLike<'a, Jsx.element>
= 'a => Jsx.element
= 'a => React.element

If there is no way to "fix" the output, then this needs to be well documented.

cristianoc commented 1 year ago

This change has the same effect:

type compType<'t> = props<'t> => React.element

@module("@foo/bar")
external make: compType<_> = "SomeComponent"

so this looks like a behaviour of external declarations.

cristianoc commented 1 year ago

Here's an even simpler example of the same phenomenon the does no use anything: no JSX, no records:

module MMM = {
  type props = string

  @module("@foo/bar")
  external make: props => React.element = "SomeComponent"

}

let ddd = React.createElement(MMM.make,  "message")
mununki commented 1 year ago

I guess this is caused by different lambda IR. Here is a simple lambda representation.

module React = {
  type element
  type compType<'props> = 'props => element
  let createElement = (comp, props) => comp(props)
}

module M = {
  type props = string

  @module("@foo/bar")
  external make: React.compType<props> = "SomeComponent"
}

module MMM = {
  type props = string

  @module("@foo/bar")
  external make: props => React.element = "SomeComponent"
}

let d = React.createElement(M.make, "message")
let ddd = React.createElement(MMM.make, "message")
(let
  (React/1002 =
     (let
       (createElement/1005 =
          (function comp/1006 props/1007 (apply comp/1006 props/1007)))
       (makeblock [createElement] createElement/1005))
   M/1008 = (makeblock [])
   MMM/1011 = (makeblock [])
   d/1014 =
     (apply (field:createElement/0 React/1002) (SomeComponent) "message")
   ddd/1015 =
     (apply (field:createElement/0 React/1002)
       (function prim/1028 stub (SomeComponent prim/1028)) "message"))
  (makeblock module/exports React/1002 M/1008 MMM/1011 d/1014 ddd/1015))

It seems that the different lambda generates different js expressions.

mununki commented 1 year ago

This is the expected output by different external declarations. It seems not an issue. I'm going to close it.

cknitt commented 1 year ago

@fhammerschmidt and I just ran into this when writing bindings for MUI 5.0. See https://github.com/cca-io/rescript-mui/pull/194/files.

I think the root cause for this issue is that the type definition for Jsx.element/React.element is incorrect:

React.component<'a>
= Jsx.component<'a>
= Jsx.componentLike<'a, Jsx.element>
= 'a => Jsx.element
= 'a => React.element

But it's simply not true that every React component is of the form 'a => React.element, i.e., a functional component. There are class-based components as well as other special cases like Fragment etc.

So actually React.component<'props>= Jsx.component<'props> would need to be an abstract type IMHO. (But I am not sure what consequences that would entail.)

/cc @mununki @cristianoc

mununki commented 1 year ago

I agree that it would make users confused. I'm curious what issues are going to be raised when we change the type definitions of props => React.element and React.component<props>.

cknitt commented 1 year ago

The JsxC/JsxU modules currently have

type componentLike<'props, 'return> = 'props => 'return
type component<'props> = componentLike<'props, element>

/* this function exists to prepare for making `component` abstract */
external component: componentLike<'props, element> => component<'props> = "%identity"

The comment suggests that there was already previously the idea of making the component type abstract.

So if we had

type componentLike<'props, 'return> = 'props => 'return
type component<'props>

external component: componentLike<'props, element> => component<'props> = "%identity"

then the component function would need to be used for conversion and the example from JSXV4.md

@react.component (~x, ~y=3+x, ~z=?) => body

would need to be transformed to

type props<'x, 'y, 'z> = {x: 'x, y?: 'y, z?: 'z}

React.component(({x, ?y, ?z}: props<_, _, _>) => {
  let y = switch y {
  | None => 3 + x
  | Some(y) => y
  }
  body
})

Right?

rickyvetter commented 1 year ago

Hey folks. I want to provide some historical context here.

Obviously React.component should be an abstract type on the JS side and so, for that reason alone, modeling it correctly in ReScript is worthwhile. But the original move toward making it abstract was because of the ReScript compiler. Having functions that return functions makes ReScript do interesting curry/application things that are often not what people intended when writing component factories. By annotating components as an alias instead of as a function it deoptimized that compiler behavior and made the code act as expected. So the alias was added because it was a much smaller breaking change and intention was to mod over to an abstract type eventually (which has tons of headaches).

Also want to mention that the annotation itself is important not just for the ReScript transform but also the couple things it does for JS debug ability. If you remove it then you lose component names in the React Dev tools and lots of stuff becomes more difficult to understand. On the external side it's not as important, but you needed it to do handle non-function components.

Edit because I was rambling. I think this is why you get the function wrapper on externals^^ it was a stopgap for issues with component factories that returned JS lib components that aren't functions. The function wrapper from @react.component makes it so all externals are functions and the type alias kinda protects against compiler optimizations.

I haven't read JSX4 changes completely (yet) so I mostly don't know what I'm talking about :) but abstract type would solve all of that and be a super annoying breaking change.

cknitt commented 1 year ago

Hi @rickyvetter, thanks for the historical context! 👍

Could you elaborate a bit on the "tons of headaches"? Sure, this would be a breaking change, but why would it be "super annoying"?

(BTW, there is already a breaking change in JSX4 for ReScript 11 / @rescript/react 0.12.0 anyway because of https://github.com/rescript-lang/rescript-compiler/pull/6238 / https://github.com/rescript-lang/rescript-react/pull/94.)

rickyvetter commented 1 year ago

It was a big social change to ask people to use React.component(...) when there was a lot of angst over string. Also the modding of externals couldn't be fully automated and was pretty awkward because of make+makeProps needing to still exist. I actually think JSX4 makes that part of it quite a bit simpler. Social part of modding extra wrappers into code bases is still rough.

mununki commented 1 year ago

Thank you for explaining @rickyvetter Now I get it what you @cknitt mentioned about the root causes. There's no reason not to try it. Let me change JSX4 to use component and see what it brings to us.

github-actions[bot] commented 17 hours ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.