Closed davesnx closed 1 month ago
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:
Belt.filterMap(id)
to remove None
valuessolves 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:
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.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.
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:
So all usages of
element_without_key
andelement_with_key
can be explicit on the bindings.@johnhaley81
Pros of this PR
Example of the type error:
Cons