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

Warning: Each child in a list should have a unique "key" prop. #6

Closed
zanaptak opened this issue Jul 24, 2019 · 12 comments
Closed

Warning: Each child in a list should have a unique "key" prop. #6

zanaptak opened this issue Jul 24, 2019 · 12 comments

Comments

@zanaptak
Copy link
Contributor

It seems that each level of nesting is always producing a Warning: Each child in a list should have a unique "key" prop. in the browser console.

In old DSL I believe this only happened with ofList helper.

Workaround is to put prop.key on every element.

Repro:

let nestedDivs (model:Model) dispatch =
    Html.div [
        prop.id "id"
        prop.className "class"
        prop.children [
            Html.text "text"
            Html.div [
                prop.id "id"
                prop.className "class"
                prop.children [
                    Html.text "text"
                    Html.div [
                        prop.id "id"
                        prop.className "class"
                        prop.children [
                            Html.text "text"
                            Html.div [
                                prop.id "id"
                                prop.className "class"
                                prop.children [
                                    Html.text "text"
                                    Html.div []
                                ]
                            ]
                        ]
                    ]
                ]
            ]
        ]
    ]

Produces:

Warning: Each child in a list should have a unique "key" prop. See https://fb.me/react-warning-keys for more information.
    in div (created by Elmish_React_Components_LazyView)
    in Elmish_React_Components_LazyView react-dom.development.js:506:32
Warning: Each child in a list should have a unique "key" prop. See https://fb.me/react-warning-keys for more information.
    in div (created by Elmish_React_Components_LazyView)
    in div (created by Elmish_React_Components_LazyView)
    in Elmish_React_Components_LazyView react-dom.development.js:506:32
Warning: Each child in a list should have a unique "key" prop. See https://fb.me/react-warning-keys for more information.
    in div (created by Elmish_React_Components_LazyView)
    in div (created by Elmish_React_Components_LazyView)
    in div (created by Elmish_React_Components_LazyView)
    in Elmish_React_Components_LazyView react-dom.development.js:506:32
Warning: Each child in a list should have a unique "key" prop. See https://fb.me/react-warning-keys for more information.
    in div (created by Elmish_React_Components_LazyView)
    in div (created by Elmish_React_Components_LazyView)
    in div (created by Elmish_React_Components_LazyView)
    in div (created by Elmish_React_Components_LazyView)
    in Elmish_React_Components_LazyView
@MangelMaxime
Copy link
Contributor

I think this is caused by (createObj [ "children" ==> [| value |] ])

Source

We should pass directly the list without any modification to Interop.reactElement

static member inline div xs = Interop.reactElement "div" xs

Code was taken from the second snippet on this comment

@Zaid-Ajaj
Copy link
Owner

Zaid-Ajaj commented Jul 24, 2019

@zanaptak Thanks for filing the issue!

@MangelMaxime I don't get it, where is the problem coming from exactly? The only thing I can think of, is that we should not generate a list of children but instead using a spread:

BAD

React.createElement(div, null, [ 
  React.createElement("h1", null, "Content"),
  React.createElement("h2", null, "More content") 
])

GOOD

React.createElement(div, null, 
  React.createElement("h1", null, "Content"),
  React.createElement("h2", null, "More content") 
)

But I am not sure how to compile a list into a spread in Fable, is that what that [<ParamList>] is doings?! (which is why we don't need to call Array.ofList on it?? cc @alfonsogarciacaro

@MangelMaxime
Copy link
Contributor

Hum, I didn't saw that there is 2 versions of all the HTML element the helpers... And saw only the one re-creating the children.

I am not sure about this 2 helpers thing. I suspect you added it here in order to not force the user to create a list if he have only one direct child. But it makes the code less consistant and refactoring harder because the code is not always the same.

@MangelMaxime
Copy link
Contributor

MangelMaxime commented Jul 25, 2019

But I am not sure how to compile a list into a spread in Fable, is that what that [] is doings?! (which is why we don't need to call Array.ofList on it?? cc @alfonsogarciacaro

Yes this is indeed what [<ParamList>] is made for

@MangelMaxime
Copy link
Contributor

@Zaid-Ajaj I can't find a way to use the spread syntax when using the children property.

It's really strange because when I dump the instance values between our version and a spread version they both don't have the key filled.

A possible solution is to use Children API from React:

React.Children.toArray

Another pretty self-explanatory utility. toArray converts the children object to an array. It also assigns a key to each child to scope them to the input array. This is useful if you need to reorder or slice the children object.

type ReactChildren =
    abstract toArray: ReactElement -> ReactElement seq
    abstract toArray: ReactElement seq -> ReactElement seq

type IReactApi =
    abstract createElement: comp: obj * props: obj -> ReactElement
    abstract createElement: comp: obj * props: obj * [<ParamList>] children: ReactElement seq -> ReactElement
    abstract Children : ReactChildren

[<Global("React")>]
let reactApi : IReactApi = jsNative

type prop =
    static member children (elems: ReactElement seq) = 
        Interop.mkAttr "children" (reactApi.Children.toArray elems)

Like that React will add the missing key for us:

let view =
    Html.div [
        // attr.className "button"
        attr.children [
            Html.div [ 
                attr.content "1"
                attr.key "custom-key"
            ]

            Html.div [ 
                attr.content "2"
            ]
        ]
    ]

gives:

Capture d’écran 2019-07-25 à 10 36 34

@alfonsogarciacaro
Copy link
Contributor

alfonsogarciacaro commented Jul 25, 2019

Yes, ParamList is equivalent to ParamArray and spreads the arguments. It should actually be ParamSeq because it accepts any seq since Fable 2 but it was like that in Fable 1 so I kept the name.

BTW, Fable needs access to the list elements at compile time for proper spreading. When not possible, it uses the JS spread operator .... This is one of the reasons all functions in Fable.React.Standard are inlined.

@Zaid-Ajaj
Copy link
Owner

Thanks for explanation @alfonsogarciacaro, this works nicely when the list is in it's own parameter but not when it is a property of a larger object because it wouldn't make sense like @MangelMaxime said:

I can't find a way to use the spread syntax when using the children property.

I was thinking of extracting the children property from props and then apply to a parameter with ParamList but I wasn't sure if Fable would beta-reduce the lookup operation all the way to a simple list spread (especially if the list has conditional rendering based on runtime information)

@MangelMaxime your solution looks great! I will give it a shot 😄

@Zaid-Ajaj
Copy link
Owner

Warnings are gone! thanks a lot @MangelMaxime

@zanaptak Can you give it a try in v0.19?

@alfonsogarciacaro
Copy link
Contributor

alfonsogarciacaro commented Jul 25, 2019

Yes, you can extract the children property before passing it to the React. A dirty way of doing that could be (note I'm removing inline because it's better not to inline so much code):

let [<Emit("$0 in $1")>] isKeyIn (key: string) (o: obj): bool = jsNative
let [<Emit("delete $1[$0]")>] deleteKeyFrom (key: string) (o: obj): unit = jsNative

module Interop =
    let createElement name (properties: IReactProperty list) : ReactElement =
        let props = keyValueList CaseRules.LowerFirst properties // Inlining only this part can help optimizations from the compiler
        if isKeyIn "children" props then
            let children = props?children
            deleteKeyFrom "children" props
            ReactBindings.React.createElement(name, props, children)
        else
            ReactBindings.React.createElement(name, props, [])

BTW, the key prop is one of the two main heuristics used by React for the tree diffing so I wouldn't just ignore it. Users should still have the possibility of adding a key when dealing with actual collections. This is important when having a large table with filters or sorting for example.

@MangelMaxime
Copy link
Contributor

@alfonsogarciacaro The solution I proposed don't ignore the key prop.

It asks to React to provide the key it's using internally if not is set.

That's why you see:

  • .$custom-key which contains the key information I provided via prop.Key "custom-key"
  • .1 which is the key used by react internally

I didn't test if there is any performance penalty by this solution but I don't think there is.

If there is we can indeed, try your proposed solution. :)

@Zaid-Ajaj
Copy link
Owner

Yeah we can try the solution Alfonso proposed, for now this is the simplest one without helper code 😄

@zanaptak
Copy link
Contributor Author

@Zaid-Ajaj Yep fixed, thanks. 👍

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

No branches or pull requests

4 participants