Explore to make JSX component abstract

[mununki]

discussion: #5725

This PR explores to make the JSX component as an abstract type. As a result of this PR change, the following type definition will cause a build error.

// build error
module M0 = {
  type props = string
  @module("A")
  external make: props => React.element = "default"
}

And now components that don't return React.element will throw a build error.

// build error
module M1 = {
  type props = {x: int}
  let make = (~x) => x
}

Note: With this change in PR, it looks like we're going to have to rewrite quite a bit of our tests. For now, I've commented out tests like /built_test/react_ppx for exploration.

[mununki]

Withouth the component application, The type of this module contains type variables that cannot be generalized: A build error occurred.

[mununki]

Should we also modify JSX3?

[cknitt]

Awesome! Thanks! Will test tomorrow.

[mununki]

@cknitt Thanks! I'll keep going through the test cases and see if I can reduce the breaking changes.

[mununki]

This change will eventually affect JSX3 as well.

[cknitt]

@mununki I tried to build one of our projects (fairly large, web + React Native) with this change.

And actually this didn't look too bad. All dependencies built successfully without any changes/patching. There were some issues in the project code itself though. I had little time today and haven't finished resolving all of them, but I have not come across a blocker yet. Will continue tomorrow.

One interesting thing was this:

@react.component
let make = (~x) =>
  switch x {
  | #a => React.string("A")
  | #b => React.string("B")
  | _ => React.string("other")
  }

=>

  This expression's type contains type variables that cannot be generalized:
  React.component<props<_[> #a | #b]>>```

[mununki]

Yes, that is one of things to be fixed in this PR. The trasformation of component definition is changed in this PR as below:

// before
type props<'x> = {x: 'x}

let make = ({x, _}: props<_>) =>
  switch x {
  | #a => React.string("A")
  | #b => React.string("B")
  | _ => React.string("other")
  }
let make = {
  let \"Abstract" = (props: props<_>) => make(props)

  \"Abstract"
}

// after
type props<'x> = {x: 'x}

let make = ({x, _}: props<_>) =>
  switch x {
  | #a => React.string("A")
  | #b => React.string("B")
  | _ => React.string("other")
  }
let make = {
  let \"Abstract" = React.component((props: props<_>) => make(props)) // wrapping with React.component

  \"Abstract"
}

This change seems to interfere with the compiler's type inference. There are a couple other cases that I'm looking at.

[cknitt]

The same thing occurs whenever there is a type variable in a prop type like in

@react.component
let make = (~x: 'x, ~render: 'x => React.element) => render(x)

I have something similar in one component, so that's blocking me now.

[cristianoc]

Is there a way to approach this gradually?
I guess the treatment of component as functions is only problematic when FFI is used? (As there's no way to create a class component from ReScript, or is there).

[cknitt]

Well, yes, but usually you will be using bindings to 3rd party libs when working with React (or writing bindings yourself), so you will be using FFI.

Even with just @rescript/react, e.g.

@react.component
let make = () => {
  React.Fragment.make({children: React.string("Boom")})
}

would compile fine and go 💥 at runtime. Maybe a contrived example, but still.

[cristianoc]

Asking in case one can special case the ffi only.

[cristianoc]

Yes, that is one of things to be fixed in this PR. The trasformation of component definition is changed in this PR as below:

// before
type props<'x> = {x: 'x}

let make = ({x, _}: props<_>) =>
  switch x {
  | #a => React.string("A")
  | #b => React.string("B")
  | _ => React.string("other")
  }
let make = {
  let \"Abstract" = (props: props<_>) => make(props)

  \"Abstract"
}

// after
type props<'x> = {x: 'x}

let make = ({x, _}: props<_>) =>
  switch x {
  | #a => React.string("A")
  | #b => React.string("B")
  | _ => React.string("other")
  }
let make = {
  let \"Abstract" = React.component((props: props<_>) => make(props)) // wrapping with React.component

  \"Abstract"
}

This change seems to interfere with the compiler's type inference. There are a couple other cases that I'm looking at.

You you can't do type inference under an application.
But, you could do this:

let make = {
  let \"Abstract" = (props: props<_>) => make(props)

   React.component(\"Abstract")
}

[mununki]

You you can't do type inference under an application. But, you could do this:

let make = {
  let \"Abstract" = (props: props<_>) => make(props)

   React.component(\"Abstract")
}

Wow! Thanks! I'll change and try it.

[mununki]

If I change the application location of the React.component as advised, I still get a build error. But if I add code that uses the component, the build succeeds regardless of the location of the React.component.

module C = {
  @react.component
  let make = (~x: 'x, ~render: 'x => React.element) => render(x)
}

// adding the component application, then build fine no matter `React.component` locations.
let _ = <C x=1 render={v => React.int(v)} />

[zth]

EDIT: Realize I posted in the wrong PR...

[mununki]

@cknitt Off the top of my head, if we were to make the type of the react component abstract, so that we could use the React.component identity function via ppx only to define the component type, we'd have one problem. We won't be able to define components like this without ppx.

module C = {
  type props = {a: string}
  let make = props => React.string(props.a)
}

So why don't we move towards making 'props => 'return' abstract only for component definitions that interop externally, as @cristianoc suggests?

[mununki]

Rebased to master

[mununki]

@mununki I tried to build one of our projects (fairly large, web + React Native) with this change.

And actually this didn't look too bad. All dependencies built successfully without any changes/patching. There were some issues in the project code itself though. I had little time today and haven't finished resolving all of them, but I have not come across a blocker yet. Will continue tomorrow.

One interesting thing was this:

@react.component
let make = (~x) =>
  switch x {
  | #a => React.string("A")
  | #b => React.string("B")
  | _ => React.string("other")
  }

=>

  This expression's type contains type variables that cannot be generalized:
  React.component<props<_[> #a | #b]>>```

I've discussed this issue with @cristianoc, we can summarize the issue to the example below:

type t<'a> = [> #a | #b] as 'a

let f = (x: t<_>) =>
  switch x {
  | #a => React.int(0)
  | #b => React.int(1)
  | _ => React.int(2)
  }

let component: ('props => 'element) => 'component = a => a

let \"H1" = component((x: t<_>) => f(x)) // error
let \"H2" = Obj.magic((x: t<_>) => f(x)) // Ok

The component approach seems not working with the type checker. The main purpose of making the jsx component as an abstract type is preventing the normal function signature from treating as a jsx component. Then, I'm thinking about a new strategy for the purpose.