reason-react

reason-react copied to clipboard

Introduced an idea where there's a difference between elements/components with keys. Useful for the type-checker to raise an issue where keys aren't passed, but React.array needs them.

The implementation of the types goes as follow:

type element('a);

type keyed; /* Flag to mark element('a) to be keyed */
type not_keyed; /* Flag to mark element('a) to be not keyed */

type element_without_key = element(not_keyed);
type element_with_key = element(keyed);

So all usages of element_without_key and element_with_key can be explicit on the bindings.

@johnhaley81

---

Pros of this PR

let root = ReactDOM.Client.createRoot(container);
let array = [|1, 2, 3|]->Array.map(item => {<div> item->React.int </div>});
ReactDOM.Client.render(root, <div> array->React.array </div>);

Example of the type error:

This expression has type array(React.element(React.not_keyed))
but an expression was expected of type array(React.element_with_key)
Type React.element(React.not_keyed) is not compatible with type
  React.element_with_key = React.element(React.keyed)
Type React.not_keyed is not compatible with type React.keyed

Cons

• Type errors are somewhat more obtuse
• Working with the codebase might become a little trickier, since you would need to know the relation's the abstract types.

I need to look at this thoroughly, but one thing that obviously becomes harder is annotating since there's the new phantom type variable for type element.

cc @mlms13 @maxkorp @andywhite37 @endlo would love your input on this if you can.

I started testing the approach in #813 (which is in the end very similar to this 😄 ) and noticed a case in reshowcase that breaks.

The code breaks here with this error:

File "src/ReshowcaseUi.re", lines 327-329, characters 21-15:
327 | .....................{
328 |                 React.null;
329 |               }
Error: This expression has type React.element
       but an expression was expected of type React.elementKeyed

The code does something like this:

  Array.map(((entityName, entity)) => {
    let searchMatchingTerms =
      HighlightTerms.getMatchingTerms(~searchString, ~entityName);

    let isEntityNameMatchSearch =
      searchString == "" || searchMatchingTerms->Belt.Array.size > 0;

    switch (entity) {
    | Demo(_) =>
      if (isEntityNameMatchSearch || parentCategoryMatchedSearch) {
        <Link
          key=entityName
          ...
        />;
      } else {
        React.null;
      }
  ...

I think the error makes sense. I am not sure what the fix should be. React supports null elements intermingled with keyed ones, and it won't show any warnings. Maybe there could be a new element nullKeyed? Which is essentially the null element but with type keyed?

I believe this is a bug in our code, right? An array of elements where some of them are null seems a bad JSX construction. I have coded myself worst atrocities and later filtering out React.nulls.

True. Splitting the processing in 2 steps:

• first to remove the null values (can use Belt.filterMap(id) to remove None values
• another to create the elements

solves the issue. See https://github.com/ahrefs/reshowcase/commit/68718ce5e7addd5e97dc5d35af56ef01c4ced3e9 for the fix (it includes another fix for unneeded key attrs at the bottom).

Another interesting case I found is React.Children.mapWithIndex and similar functions, e.g.

[@react.component]
let make = (~children) =>
  <ol>
    {children->React.Children.mapWithIndex((element, index) =>
       <li key={string_of_int(index)}> element </li>
     )}
  </ol>;

I fixed it in https://github.com/reasonml/reason-react/commit/29fc9be70078036f476e63b0993a81c12ec923d3.

---

I spent some time applying the typed keys to ahrefs monorepo, I am far from finished but I found already 5 bugs of elements that were missing keys. There were also 10+ places where keys were added unnecessarily.

I think the largest pain points are:

• Missing React.null external with type elementKeyed. I ended up creating one manually for simplicity, maybe reason-react could provide a React.keyedNull external with a deprecation notice, at least for migration purposes.
• In a few cases, a function would create keyed elements, that were being used both as part of array maps (need keys) or just as plain literal elements (doesn't need keys). If the approach in #813 is used, it requires splitting the function into 2: one with keyed elements and one without
• Sometimes the locations for the errors when a children has keys when it shouldn't are too broad, but it's just a matter of checking the children to see which one has key prop.

In general, my impression is that this improvement could be very useful, and once folks get used to how the types work, the advantages would overcome the downsides. @davesnx @johnhaley81 thanks for proposing it! ❤️

Ah, another thing I forgot is that React.Children is quite problematic. I was reading about it on the React official docs and they suggest again using anything in that module, so maybe it could be deprecated eventually?

https://react.dev/reference/react/Children

There were also 10+ places where keys were added unnecessarily

That's very suspicious, people rarely add keys without a good reason.