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

ref vs key #129

Closed
bryceosterhaus opened this issue Jun 16, 2016 · 9 comments
Closed

ref vs key #129

bryceosterhaus opened this issue Jun 16, 2016 · 9 comments

Comments

@bryceosterhaus
Copy link
Member

I have been getting a little mixed up on the differences between these two and when to use each.

(1) // This makes sense
<div>
     {
          items.map(
                 item => (
                        <span key={item.id}>{item.name}</span>
                 )
          )
     }
</div>

(2) // Not sure about this use case
<div>
     {
          items.map(
                 item => (
                        <SomeComponent data={item} key={item.id} />

                        // Or do I use `ref` since its a component?
                        <SomeComponent data={item} ref={item.id} />
                 )
          )
     }
</div>


(3) // And this? should I use a ref or key?
<div>
     <SomeComponent data={item1} key={item1.id} />

     {someBool &&
           <SomeComponent data={item2} key={item2.id} />
      }

     <SomeComponent data={item3} key={item3.id} />
</div>


(4) // And why can't I use ref's for referencing an element?
someFunction() {
     this.components.container // wanting to reference the container node
     // I would have to use this.component.element.querySelector() instead

     this.components.component1 // this works
}

render() {
    return (
          <div ref="container">
              <SomeComponent ref="component1" />
         </div>
    )
}

So those are a few cases above that seem strange.

I understand that this.components provides reference to the instance of the child components but I would expect to be able to reference any element in that manner.

The biggest confusion for me is example (2). When iterating through a list and rendering multiple components, should I use key or ref? For a standard html element, I would always use key.

From using metal, I would hope to be able to use ref to reference both elements and components. And then for key I would only use that when I am iterating inside of a parent component. Which also brings to mind this previous conversation. I am starting to think more so that components should be keyed based on their parent element, rather than their parent component.

This is more so an open-ended issue, not necessarily a bug. Just a little confused on API and hoping to either clarify the actual API or set some rules on how to use key vs ref.

Let me know what you think! Thanks

@mthadley
Copy link

If the intention is to match React's API, then I would think it should be something like this:

When returning an array of elements in a JSX expression, make sure they each have a key config. ref would only serve to provide a reference to an element. So there could be cases where an element has both a key and a ref as part of it's config.

element in this case can be either a <div /> or <MyFancyComponent />. The value of a ref to these would be a DOM node and a component instance, respectively.

P.S. I'm not saying this IS currently how the API works, but it's what I would expect if I was a React developer looking at Metal.

@mairatma
Copy link
Contributor

@mthadley you got it right, that's exactly it. Basically key is used to help the rendering algorithm to reuse elements better within a given parent element, so you'd usually use it inside loops, regardless of if the children are elements or components. ref on the other hand is only used to identify the instance of a given component tag. It also helps us know better which instance to reuse for each call, regardless if they're rendered in the same parent or not.

As for example number 4, I hadn't thought of this use case yet. Is this something that React allows too? It does make sense for me that you'd receive the reference to the element in that case, but then we couldn't put that inside this.components, since it's not a component. Maybe we'd need to rename this variable to something like this.refs if we add this feature (probably maintaining the old this.components as well though, for backwards compatibility).

@bryceosterhaus
Copy link
Member Author

Yes, react currently does allow refs for elements and they specify all refs on this.refs. If it is an element you would get the node back, if it is a component than you would get the component instance.

So for iterating through multiple components, do we use both ref and key?

<div>
     {
          items.map(
                 item => (
                        <SomeComponent data={item} key={item.id} ref={item.id} />
                 )
          )
     }
</div>

@mairatma
Copy link
Contributor

If it's just an iteration you'd only need key. ref is safer to guarantee that we match the right instances if you conditionally render some of these components. Since the order of these can change I think we might match instances wrongly without ref in this case though... I think you're right that we might need to change the matching logic to be based on parent element with keys, so that ref isn't necessary in cases as common as this. I'll start looking into how much work this change would require.

@bryceosterhaus
Copy link
Member Author

we might match instances wrongly without ref in this case though

I think this is something we are running into right now, though its not a huge issue currently because we can use key's as well. But in the future I can see it becoming an issue. I think changing matching logic to be based on parent elements would be of great benefit.

Let us know what you find out and let us know if we can help in anyway.

@mairatma
Copy link
Contributor

mairatma commented Jul 5, 2016

This issue was moved to #5

@mairatma mairatma closed this as completed Jul 5, 2016
@mairatma
Copy link
Contributor

This issue was moved back.

@mairatma mairatma reopened this Jul 21, 2016
@mairatma
Copy link
Contributor

I've finished this change, it's available on version 2.0.0 now.

@gupei515
Copy link

the answer is exactly what I am looking for! thanks @mthadley !

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