typed keys 2

[jchavarri]

Alternative take to having typed keys based on the ideas from #812.

Centers the typing around the React.array function. The only case where keys should be added is when children are variable size arrays, and not literals. So React.array might be the best spot to place the restriction.

The issue is that React.array is used in the ppx to convert literals of children like <div> foo bar </div>. To cover that case, a new function React.unsafeArray is added.

The type errors look like:

File "test/React__test.re", line 140, characters 41-46:
140 |       ReactDOM.Client.render(root, <div> array->React.array </div>)
                                               ^^^^^
Error: This expression has type React.element array
       but an expression was expected of type React.elementKeyed array
       Type React.element is not compatible with type React.elementKeyed

One downside is that usages of key outside arrays now would trigger a type error. This makes sense, because why would key be added if no needed, but who knows what could break. Also, in all cases the error happens far away from where the component is defined.

E.g. for this code:

module Foo = {
  [@react.component]
  let make = () => {
    <div key="hey"> 2->React.int </div>;
  };
};

let t = <div> <Foo /> </div>;

the error is:

File "test/React__test.re", line 376, characters 14-21:
376 | let t = <div> <Foo /> </div>;
                    ^^^^^^^
Error: This expression has type <  > Js.t -> React.elementKeyed
       but an expression was expected of type
         <  > Js.t React.component = <  > Js.t -> React.element
       Type React.elementKeyed is not compatible with type React.element

[anmonteiro]

I guess this exposes the downside you talked about in the description.

I think this is a pretty big limitation, as you can't write this anymore:

module Foo = {
  [@react.component]
  let make = (~id) => {
    <div key=id />;
  };
};

module Bar = {
  [@react.component]
  let make = () => {
    let ks = Array.init(10, string_of_int);
    <> {Array.map(ks, id => <Foo id />)} </>;
  };
};

Check the error:

379 |     <> {Array.map(ks, id => <Foo id />)} </>;
                                  ^^^^^^^^^^
Error: This expression has type < id : string > Js.t -> React.elementKeyed
       but an expression was expected of type
         < id : string > Js.t React.component =
           < id : string > Js.t -> React.element
       Type React.elementKeyed is not compatible with type React.element

[jchavarri]

I believe the keys have to be added directly in the elements that are part of the list, so the keys added to the div in Foo in your example are useless.

Check the console warnings from React in this JS snippet, adapted from yours:

https://codesandbox.io/s/modest-http-dspy7k

[jchavarri]

This compiles fine, and it'll guarantee the keys are passed correctly to React:

module Foo = {
  [@react.component]
  let make = () => {
    <div />;
  };
};

module Bar = {
  [@react.component]
  let make = () => {
    let ks = Array.init(10, string_of_int);
    <div> {Array.map(ks, id => <Foo key=id />)->React.array} </div>;
  };
};

[davesnx]

Indeed, key needs to be on the call-site: <Foo id /> -> <Foo id key="" />

[andreypopp]

This makes sense, because why would key be added if no needed

Sometimes it's useful to have key specified outside of an array to re-mount the element. Ref to React docs: https://react.dev/reference/react/useState#resetting-state-with-a-key

[johnhaley81]

Having element and elementKeyed as a completely different type was the main problem that @davesnx and I were running into. There are legit use-cases for supplying a key to an element that is outside of an array and to prevent that at this level would be wrong IMO.

[jchavarri]

There are legit use-cases for supplying a key to an element that is outside of an array

Can you share some specific examples please? I could only find 1 example on the whole Ahrefs codebase, of a function that was generating some element, and it could be easily fixed.

to prevent that at this level would be wrong IMO.

To me it's more a line of tradeoffs between approaches than right or wrong. Adding a type variable to element has some implications as well in terms of error messages, type complexity, and general dev experience. It also will break more existing code. Some of the impact can be seen already in #812, where a lot more code has to be modified. I can imagine several 3rd party libs out there expect element to be a type without variables.

I would love to see the approach on #812 applied on Ahrefs monorepo so that we can have more information about how a migration would look like (both for the monorepo code and the opam libs) and how the errors will show with element('a). That way, we can compare both approaches in a more complete way?

[johnhaley81]

Can you share some specific examples please? I could only find 1 example on the whole Ahrefs codebase, of a function that was generating some element, and it could be easily fixed.

The most common usages of key in React that I have used (outside of arrays) are:

• to completely re-initialize a component which will clear the state, for example an <input> should be tied to a stateful value from its parent and reset if that state is changed.
• help rendering performance for components with many children that are dynamic, for example if you have a component where it has static children that are rendered or not rendered based on some value and those children are the same element type (i.e. all <div>) then React might cause unnecessary re-renders when a components sibling changes.

I would love to see the approach on #812 applied on Ahrefs monorepo so that we can have more information about how a migration would look like (both for the monorepo code and the opam libs) and how the errors will show with element('a). That way, we can compare both approaches in a more complete way?

Yeah! This is a great call and I can try and whip something up for you!

[johnhaley81]

@jchavarri I've been hitting enough blockers to try and get a PoC going for you today that I'm going to take a little break and come back to it later. I did a few things on my fork of the PR as well if you want to check them out.