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

POC of giving a type-error for non keyed elements when are under React.array #812

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

davesnx
Copy link
Member

@davesnx davesnx commented Nov 2, 2023

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.

type component('props) = componentLike('props, element);
type element('a); /* 'a captures any type information to differenciate with others 'a's */

type keyed; /* Flag to mark element('a) to be keyed */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a note, I think this won't work correctly after ocaml/ocaml#8900

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally, we can workaround it by adding "private" fields:

type keyed = private Tag_keyed
type non_keyed = private Tag_non_keyed

@anmonteiro
Copy link
Member

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.

@johnhaley81
Copy link

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

@jchavarri jchavarri mentioned this pull request Nov 3, 2023
@jchavarri
Copy link
Collaborator

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?

@davesnx
Copy link
Member Author

davesnx commented Nov 3, 2023

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.

@jchavarri
Copy link
Collaborator

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 ahrefs/reshowcase@68718ce for the fix (it includes another fix for unneeded key attrs at the bottom).

@jchavarri
Copy link
Collaborator

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 29fc9be.


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 typed keys 2 #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! ❤️

@jchavarri
Copy link
Collaborator

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

image

@davesnx
Copy link
Member Author

davesnx commented Nov 3, 2023

There were also 10+ places where keys were added unnecessarily

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

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

Successfully merging this pull request may close these issues.

None yet

4 participants