japgolly / scalajs-react

Facebook's React on Scala.JS
https://japgolly.github.io/scalajs-react/
Apache License 2.0
1.64k stars 232 forks source link

Key not getting passed in children array for ReactComponents #6

Closed dispalt closed 10 years ago

dispalt commented 10 years ago

somewhere between 0.2.0 and 0.2.1-SNAPSHOT something got lost with propagating the key property. Take a look at this example, react says I am not passing the key property through.

val ApplicationItem = ReactComponentB[PApplication]("ApplicationItem")
    .render(P => {
      tr(key := P.id)(td(P.name), td(P.description), td(P.url))
    }).create

....
val App = ReactComponentB[Seq[PApplication]]("ApplicationTable")
    .initialState(ApplicationStore.allApps())
    .backend(new AppBackend(_))
    .render { (P, S, B) =>
      val body: Modifier = if (S.isEmpty) tr(key := "empty-state")(td(colSpan := "3")("Empty!"))
      else S.map(f => ApplicationItem(f))

      div(cn := "block-flat")(
        div(cn := "content")(
          table(cn := "no-border table-condensed")(
            thead(cn := "no-border")(
              tr(th("Name"), th("Description"), th("URL"))
            ),
            tbody(cn := "no-border-x no-border-y")(
              body
            )
          )
        )
      )
    }.create

Now if I do this, it works. This is a pretty common pattern, but I am not entirely sure how to fix it, there is a lot of implicits to track down to figure out what's going on.

val App = ReactComponentB[Seq[PApplication]]("ApplicationTable")
    .initialState(ApplicationStore.allApps())
    .backend(new AppBackend(_))
    .render { (P, S, B) =>
      val body: Modifier = if (S.isEmpty) tr(key := "empty-state")(td(colSpan := "3")("Empty!"))
      else S.map(f => tr(key := f.id)(td(f.name), td(f.description), td(f.url)))

      div(cn := "block-flat")(
        div(cn := "content")(
          table(cn := "no-border table-condensed")(
            thead(cn := "no-border")(
              tr(th("Name"), th("Description"), th("URL"))
            ),
            tbody(cn := "no-border-x no-border-y")(
              body
            )
          )
        )
      )
    }.create

Thanks for taking a look/giving advice.

japgolly commented 10 years ago

Could you paste the error message you're seeing?

dispalt commented 10 years ago

I did some more digging and it's because we wrap the props value { val v: A }, so nothing on the object gets directly exposed to be picked up by the child. I am honestly not sure how this ever worked, it probably didn't.

Each child in an array should have a unique "key" prop. Check the render method of ApplicationTable. See http://fb.me/react-warning-keys for more information.

japgolly commented 10 years ago

Ah, try S.map(f => ApplicationItem(f)).toJsArray and make sure your checkout of this lib includes 5cdd6e0764e6d212dc273d7f4712f3d7f7f40ccb.

dispalt commented 10 years ago

I tried that a couple times, couldn't get it to compile. My understanding is that function is oriented at dom elements vs reactcomponents

I get an error something like this:

Cannot prove that japgolly.scalajs.react.ReactComponentU[fe.models.PApplication,Unit,Unit] =:= japgolly.scalajs.react.vdom.ReactVDom.TypedTag[japgolly.scalajs.react.vdom.ReactOutput].
dispalt commented 10 years ago

If I do something like this (warning ugly) it works:

      S.map { f =>
        val ai = ApplicationItem(f)
        ai.asInstanceOf[js.Dynamic].props.key = f.id
        ai
      }
dispalt commented 10 years ago

A good example of this pattern is here: http://jsfiddle.net/3Vs3Q/light/

japgolly commented 10 years ago

Can you humour me and try .asJsArray instead of .toJsArray?

Separately though, yeah there's no mechanism to set .props.key yet. If it's necessary, your hack will have to do for now and I'll put some friendly support in soon.

dispalt commented 10 years ago

I tried that too, tbh I am having some trouble following the types, I kinda feel like I still don't grasp it all yet.

It seems that there is some special handling for certain props (children, key) that are messing up the model. Would it make sense to just flatten out the props directly on the object? Then that would give the niceties of being able to use JSExport to map directly to fields... Separately I am also running into issues not being able to easily put functions on the spec directly =) Especially if mixins expect a function to be somewhere.

japgolly commented 10 years ago

Yeah, I'm a similar boat :grinning: I'm don't even know what JSExport is or does, sounds promising though! I'll have a proper look at that over the weekend (thanks for pointing it out).

Most of what we're talking about here seems to come under the interop umbrella. Scala components talking to Scala components wouldn't use React mixins for example. That whole interacting-with-JS-components space hasn't received much attention yet. It will eventually; I'll probably even start using react-bootstrap myself once I start using this stuff for real.

To get back on topic though, cheers for the demo fiddle. I'll give that a look over the weekend.

japgolly commented 10 years ago

Hey Dan, just pushed a commit for this, you can now give a component a key when you create it and under the covers, it gets put on this.props.key.

dispalt commented 10 years ago

Works perfect thanks!

japgolly commented 10 years ago

Great to hear it

chandu0101 commented 10 years ago

Hi , i am facing similar issue in 0.5.1

here is my code

  val ProductCategoryRow = ReactComponentB[String]("ProductCateoryRow")
    .render(cateory => tr(key := cateory)(
    th(cateory)
  )) .build

val ProductTable = ReactComponentB[List[Product]]("ProductTable")
    .render(products => {
    val rows = products.map(p => ProductCategoryRow(p.category))
    table(
      thead(
        tr(
          th("Name"),
          th("Price")
        )
      ),
      tbody(
        rows
      )
    )
  }).build

I tired toJSArray and asJSArray , but still key is not added to child component

Each child in an array should have a unique "key" prop. Check the render method of ProductTable. See http://fb.me/react-warning-keys for more information. 

Thanks in advance

japgolly commented 10 years ago

Hi @chandu0101, I believe the info in #27 should solve your problem. Give that try and let me know how it goes.

chandu0101 commented 10 years ago

it worked thank you :)