[Call for help] Test the React JSX v4

module TestMemoCustomComparePropsOld = {
  let memo = React.memoCustomCompareProps(
    _,
    (nextProps, currentProps) => nextProps.foo == currentProps.foo,
  )

  @react.component
  and make = memo((~foo: string) => React.string(foo))
}

I’m not used to memo, so not sure. Is it workable?

Yes, this works :+1:

1 Like

Is there a tag / published npm version we can use to test (both for the compiler and rescript-react) instead of #master? My build system has issues with GitHub references.

Not yet. Is it possible to install it from the local file system? If so, how about installing it after cloning both compiler and React binding into your local.

I can use #master locally without issues, but our (Nix-based) CI has trouble with GitHub dependencies.

Really cool! Thanks for all the great work so far, I think this definitely has the potential to make life easier! I played with the example repo you set-up a bit (really helpful to get started) and have a few questions:

I think having props be a record type is great and makes it easier to use in different places, but looking at it closer it also seems like it’s a record without any concrete types, so from a type system perspective its usefulness is very limited.

Prop types

An example snippet:

module Test = {
  @react.component
  let make = (~s) => React.string(s)
}

@react.component
let make = () => {
  let str : Test.props<int> = {
    s: 1
  };

  <Test s={str.s} />
}

This produces the following error:

32 ┆ };
33 ┆ 
34 ┆ <Test s={str.s} />

This has type: int
Somewhere wanted: string

You can convert int to string with Belt.Int.toString.

I would’ve expected the error not to be on s={str.s} but on the definition of str to begin with (and would expect to be able to use Test.props without arguments to the type constructor). Having the props as a record with abstract types isn’t very helpful if you want to create functions that prepare or deal with those prop records :smiley:

Prop spread

Is it correct that this release does not yet allow us to do

module Test = {
  @react.component
  let make = (~s) => React.string(s)
}

@react.component
let make = () => {
  let str : Test.props<string> = {
    s: "String"
  };

  <Test {...str} />
}

The compiler doesn’t complain about the propspread syntax in the component but it does say that s is not present, which is confusing:


  32 │   };
  33 │ 
  34 │   <Test {...str} />
  35 │ }
  36 │ 

  Some record fields are undefined: s

Error message

Can we improve the error message that is being provided for missing properties?


module Test = {
  @react.component
  let make = (~str, ~int) => React.string(str ++ " " ++ (int->Belt.Int.toString))
}

@react.component
let make = () => {
  <Test />
}

Provides the following error message and we can debate whether it’s better or worse than what we had.

  28 │ @react.component
  29 │ let make = () => {
  30 │   <Test />
  31 │ }
  32 │ 

  Some record fields are undefined: str

It’s better because it doesn’t provide the “expected function of type” and shows the entire makeProps function, which was often unhelpful to newcomers.

However, I think it’s also worse in a few important ways:

  1. The error message requires knowledge of the internal implementation of JSX. I don’t know that a JSX element is represented by a record, there’s no record in my code. Why is a record field missing?
  2. I only see a single missing required field at a time. This means I need to go back 'n forth with the compiler a few times, rather than being made aware of all missing properties.

Thanks again for all the great work so far! I hope we can use the above feedback to make this even better :smiley:

4 Likes

Props type

Very good point. There are two reasons in having type parameters for props type. For example,

// implementation
@react.component
let make = (~s) => React.string(s)
// The first reason
// the ppx doesn't know what type s is in the parsetree.
// Therefore, the ppx can make only type props<'s> = {s: 's}

// interface
@react.component
let make = (~s: string) => React.element
// The second reason
// If the ppx makes type props = {s: string}
// There is inconsistency between implementation and interface. 

That is why the compiler doesn’t complain about Test.props<int>.

Prop spread

I don’t think you can remember my first suggestion was. It was the spread props. That was a beginning to contribute the React JSX v4 now. :smiley: Yes, the spread props is available, but there is a restriction. Let me show you the example.

module Test = {
  @react.component
  let make = (~s, ~x) => (s ++ x)->React.string
}

@react.component
let make = () => {
  let str: Test.props<string, string> = {
    s: "s",
    x: "x",
  }

  <Test x="y" {...str} />
}

As you well know, the type of props is the record now in v4. Actually, the spread props is using the spread operator ... in the record. So, another prop is needed at least such as x. <Test x="y" {...str} /> will be transformed roughly.

React.createElement(Test.make, {x: "y", ...str})

Error message

Very good point. We need to improve the error message from the compiler and also the type information from the editor about props type.

I hope my answers helpful for you. Thank you for your feedback!

1 Like

@moondaddi would you add reminder issues for these things, whether fixes or improved docs?

Sure, I’ll add it. :+1:

What if types were required in the implementation? Then it would know. And you could still allow _ to have the whole or parts of a type inferred.

Edit: I suppose it wouldn’t even have to be required. The ppx could just use what type information is present, and leave the rest as type variables.

How about just putting str as a whole instead of spreading when we have no other props specified? That’d solve the problem I guess.

Not sure I understand your intention correctly. If the below example is your intention, I’m afraid that it seems not compiled.

// implementation
@react.component
let make = (~a, ~b: string) => { ... }
// type props<'a, _> = { a: 'a, b: string }

// interface
@react.component
let make = (~a:int, ~b:string) => React.element
// type props<_, _> = { a: int, b: string }

Can you show an example for me to get your point?

Good idea. Why didn’t I come up with that? :sweat_smile:
I’ll give it a try and we’ll see how it goes.

2 Likes

What do you need specifically? Just a tag? Or a specific commit?

The smarter the ppx tries to be, the more surprises can happen with little code changes.
Also, giving type annotations does not guarantee a monomorphic type (e.g. (~x:_)).

There are several things one can try if this becomes an everyday problem. Notice also that since the PPX is intended to be completely transparent, one can just write the component definition manually as an escape hatch in extreme cases.

In this specific example, one can simply avoid naming the prop type:

@react.component
let make = () => {
  let str  = {
    Test.s: 1
  };

A (beta?) version published to the npm package registry instead of a GitHub reference.

It is not crucial for my ability to test (I can do it locally) but it would be nice.

I don’t know much about the details of how this works, but I was thinking more along the lines of:

// implementation
@react.component
let make = (~a, ~b: string) => { ... }
// type props<'a> = { a: 'a, b: string }

// interface
@react.component
let make = (~a:int, ~b:string) => React.element
// type props<'a> = { a: 'a, b: string }

But I see now that this would still require the interface knowing how the implementation is specified, which wouldn’t work anyway.

I was thinking that would be a way to opt in to the current behavior, but that would also require walking the entire type to find these holes and type variables, and would make it significantly more complex. And because the type variables need to be arguments on the props type itself, the interface would also need to know how the implementation is specified. So doesn’t seem possible in any case.

I humbly withdraw my proposal :slight_smile:

I think if this becomes a problem, then perhaps one can let the user define the props type directly and simply pass its name as argument to @react.component or something. Also, this could be useful in case of components that need to share the same prop type (by default, each definition is different – nominal typing).
There are several things one can try, depending on what pain points look like in everyday use. We’ll need to learn a bit about pain points once people use this more extensively.

3 Likes

Side note: I’m having the thanksgiving holiday around this weekend. I couldn’t make enough time to look into the feedback in depth. I’ll come back next week and see each in detail!

3 Likes