prop.children suppresses development-time warning about missing keys

[zaid-ajaj]

prop.children suppresses development-time warning about missing keys

[zanaptak]

Addressing this with documentation as a developer responsibility seems reasonable to me, given the situational nature for when custom keys are required, and it is non-fatal to omit them (performance impact also situational based on list size, memo usage, etc.).

[zanaptak]

Without this, we get the key warning on every multi-child element in the entire app (see #6), which is arguably worse than no warnings as it becomes overwhelming noise to the dev experience at that point, and it would be unnecessary boilerplate to have to put manual keys on everything.

However it could be argued that the warning is useful in many cases. If the user is adding/removing elements conditionally, or reordering them, then it would be nice to have the warning since those are the situations where you want a deliberate key to assist the diffing algorithm. But how would a library know when to provide the warning and when not?

With the old DSL there was the recommendation to always use ofList instead of yield!, and ofList always gives the warning to use keys. However, now there is some hurdle in using ofList (need to mix DSLs), and if you do use it, it no longer produces the warning. Users might be inclined to return to using yield!. So in some sense maybe it is an overall regression to hide the warning even despite all the noise. Maybe an ofList equivalent needs to be brought into this library.

Or maybe there could be additional helpers to nudge users into correct key usage. For example, what if the warning was brought back to prop.children by default, but also provide a prop.staticChildren (or some better name) that auto-generates the keys to remove the warning. Now, the user can explicitly choose how to implement the keys. If they have static list of elements that won't need diff optimization. they can make a deliberate choice to auto-generate keys with staticChildren. But, otherwise the "normal" prop.children will remind them to use keys for their dynamic children, and maybe even yield! is safe to use again because now they are adding keys where necessary.

[cmeeren]

Or maybe there could be additional helpers to nudge users into correct key usage. For example, what if the warning was brought back to prop.children by default, but also provide a prop.staticChildren (or some better name) that auto-generates the keys to remove the warning. Now, the user can explicitly choose how to implement the keys. If they have static list of elements that won't need diff optimization. they can make a deliberate choice to auto-generate keys with staticChildren. But, otherwise the "normal" prop.children will remind them to use keys for their dynamic children, and maybe even yield! is safe to use again because now they are adding keys where necessary.

I like this. 👍

(Though I'm still fuzzy on the React children internals, so I'm not claiming this is necessarily the best solution. It may be, I just don't know.)

[MangelMaxime]

When passing a list of children via the property children react complains about missing key. So the user will always need to provide the key for the children even if they are "static".

When using JSX or Fable.React, we don't have this "problem" because we call createElement in a spread manner.

In the first version, of Feliz we had specials helpers for extracting the children but it was increasing the bundle size and also could reduce the performance on a complex application. For now reactApi.Children.toArray has been chosen and it's an implementation detail from the consumer because you can still provide a custom key if you want and React will use it.

Feliz is a thin wrapper on top of React so I would advice to not add extra layer like staticChildren which would just confuse the user.

[cmeeren]

Feliz is a thin wrapper on top of React so I would advice to not add extra layer like staticChildren which would just confuse the user.

As a user who is not a React expect, I really appreciate warnings such as the missing key warning, which the current children prop suppresses. So there's a balance to be struck. It would be nice if Feliz had a solution to this, though I'm not saying it has to be staticChildren.

[MangelMaxime]

Well, we can always add a check when not generating production code using:

#if DEBUG
// ...
#else
// ...
#endif

it will not impact the production bundle size.

[cmeeren]

As I understand it, the problem is that some children should be able to be key-less without warning (statically defined children - the spread children you talked about in the standard Elmish.Fable syntax?), whereas others (e.g. generated from list items in the model) should have the warning. It's not about production vs. debug.

Or put another way; it's not a check to add, it's just whether to use Children.toArray or not, and a single app needs to be able to choose.

Please tell me if I'm wrong.

[MangelMaxime]

When passing children via props because are passing a list of children react ask us to provide a key to each child that's while we are using Children.toArray. Children.toArray make react explicitly set the computed the key it used internally.

When using "spread children", react seems to be treating this particular construction which uses spread createElement in a particular which avoid the need to set a key to each child.

My proposition is just to simulate the key check warning when DEBUG compiler directive is set (if I remember correctly Fable is already adding it for us).

We just need to identify which case needs to be checked, in general, it's when the user is passing a list of children using ofList, ofArray

We would still have to call Children.toArray internally but this is an implementation detail, and the user doesn't need to worry about it.

[cmeeren]

I see, thanks for clarifying.

Another option entirely is to bring back the React child syntax. Probably not desirable given the Feliz readme contains this:

Consistent formatting: no more awkward indentation using two lists for every element.

For the record, I personally haven't found that troublesome, I just format it as:

div [
  props
] [
  children
]

I.e., the only difference between Feliz and React formatted in this way is the extra line of ] [, which I don't mind - in fact, it's not a bad thing to have a clear separator between props and children.

[cmeeren]

Oh, by the way:

We just need to identify which case needs to be checked, in general, it's when the user is passing a list of children using ofList, ofArray

Not to put the question in a weird way, but why would users use ofList or ofArray? What would motivate them to do that, instead of just passing a list to prop.children and not use ofList and ofArray? After all, prop.children (naturally) contains an overload for ReactElement seq, so I don't see where ofList/ofArray is needed.

I mean, if the only reason to use ofList/ofArray is to get the warning, then if devs remember to use ofList/ofArray, they could just as well just remember to use key, right?

Come to think of it, I'm even wondering about this in the context of plain Fable.React (not Feliz), so I may have missed something here.

[MangelMaxime]

I didn't remember if we had an overload for ReactElement seq or not. But seems logic to have, well then we can add the check only under this overload I guess. Still, need to be checked.

Also, you could want to use ofList for the same reason you use it when using Fable.React. ofList and ofArray are a way to help React optimize the rendering. You can learn more by watching a talk about React performance in a Fabulous world from FableConf 2018 (If I remember the title correctly).

[cmeeren]

then we can add the check only under this overload I guess

Wouldn't that cause the warning to always fire when using children?

Also, you could want to use ofList for the same reason you use it when using Fable.React. ofList and ofArray are a way to help React optimize the rendering. You can learn more by watching a talk about React performance in a Fabulous world from FableConf 2018 (If I remember the title correctly).

Thanks! I guess it's this one? I think I've watched it, but I've evidently forgotten enough that I should watch it again. 😅

[Zaid-Ajaj]

Let's take a step back and look at the question:

"Why React.Children.toArray in prop.children?" Because the children are always constructed dynamically and the compiler cannot statically resolve the children as a spread parameter so it is necessary to have and otherwise you would get a false alarm for the warning when we only have static elements.

The issue should actually be: "prop.children suppresses development-time warning about missing keys" I am not sure if I am able to do anything about it if I want to keep this syntax as is which I definitely do. One "solution" is properly document how to use prop.children right in the XML docs of the function explaining the pros and cons

I have actually rewatched the talk about react performance but I couldn't find why ofArray and ofList are more performant because they aren't doing (1) anything (2) but that looks irrelevant to the issue we have here,

I will try to think of alternatives, for now let's keep this one open here

[MangelMaxime]

Sorry, the discussion in the talk was about using ofList instead of yield!. So let's remove that from, the current discussion.

[cmeeren]

The issue should actually be: "prop.children suppresses development-time warning about missing keys"

I agree, that seems like the core issue here.

[lukaszkrzywizna]

@MangelMaxime, although this discussion dates back a bit, I revisited the topic last week and came up with a question regarding a potential solution. Is it feasible to develop a Fable plugin that analyzes prop.children usage to identify if yield! (or any other dynamic list operation) is utilized? If so, the plugin would omit adding React.Children.toArray. Conversely, in scenarios where the composition is statically known, the plugin would include it. This approach aims to address the issue of generating unnecessary React missing keys warnings.

[MangelMaxime]

@lukaszkrzywizna I am unsure what information we have in the Fable plugin at this stage.

Someone will need to dig into the AST that we have access to and if we can do something about it or not.