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

External binding to component without @react.component #5725

Open
mununki opened this issue Oct 9, 2022 · 15 comments
Open

External binding to component without @react.component #5725

mununki opened this issue Oct 9, 2022 · 15 comments

Comments

@mununki
Copy link
Member

mununki commented Oct 9, 2022

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 mununki changed the title external binding to component without @react.component External binding to component without @react.component Oct 9, 2022
@mununki
Copy link
Member Author

mununki commented Oct 9, 2022

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
Copy link
Member Author

mununki commented Oct 9, 2022

Encouraged binding for external seems:

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

@cknitt
Copy link
Member

cknitt commented Oct 9, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member Author

mununki commented Oct 11, 2022

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
Copy link
Member Author

mununki commented Oct 11, 2022

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

@mununki mununki closed this as completed Oct 11, 2022
@cknitt
Copy link
Member

cknitt commented Jun 16, 2023

@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

@cknitt cknitt reopened this Jun 16, 2023
@mununki
Copy link
Member Author

mununki commented Jun 17, 2023

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
Copy link
Member

cknitt commented Jun 17, 2023

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
Copy link
Contributor

rickyvetter commented Jun 18, 2023

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
Copy link
Member

cknitt commented Jun 18, 2023

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 #6238 / rescript-lang/rescript-react#94.)

@rickyvetter
Copy link
Contributor

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
Copy link
Member Author

mununki commented Jun 18, 2023

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.

Copy link

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.

@github-actions github-actions bot added the stale Old issues that went stale label Sep 28, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 8, 2024
@cknitt cknitt reopened this Jan 31, 2025
@github-actions github-actions bot removed the stale Old issues that went stale label Feb 1, 2025
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

4 participants