Skip to content
This repository has been archived by the owner on Sep 5, 2020. It is now read-only.

"Missing 'key' prop for element in iterator" with metal.js #16

Closed
bryceosterhaus opened this issue Jun 14, 2016 · 7 comments
Closed

"Missing 'key' prop for element in iterator" with metal.js #16

bryceosterhaus opened this issue Jun 14, 2016 · 7 comments

Comments

@bryceosterhaus
Copy link
Member

In Metal.js we use "refs" for components when iterating through rather than a key. The linter should check for either ref or key for metal.js comonents. Metal will internally set key to ref for components.

// For component
array.map(
     (item, i) => (
           <MetalComponent ref={i} />
     )
)

// For element
array.map(
     (item, i) => (
           <div key={i} />
     )
)
@natecavanaugh
Copy link
Contributor

Thanks for filing the issue Bryce.

I've been thinking this over for a while since the port over to metal, so my initial idea was to allow that to be defined with a custom rule, such as:

"csf-jsx-key": [2, "ref"]

But are you saying you guys need to support both?

Meaning, should it look for the existence of any "key" attribute, or do you only need to support one?

@bryceosterhaus
Copy link
Member Author

We would need to support both, and if possible warn of each unique instance.

If possible the warnings would emit for

// For component
array.map(
     (item, i) => (
// no warning
           <MetalComponent ref={i} />

// warn that key is being passed instead of ref
           <MetalComponent key={i} />

// warn that no ref is being passes
           <MetalComponent />
     )
)

// For element
array.map(
     (item, i) => (
// no warning
          <div key={i} />
// warn here that ref is being used instead of key
           <div ref={i} />

// warn here that key is not being passed
           <div  />
     )
)

let me know if that makes sense

@natecavanaugh
Copy link
Contributor

natecavanaugh commented Jun 15, 2016

Hmm... okay, I think I'm a bit confused.

Couldn't both attributes be passed to both elements and components at the same time?

Also, are ref's required on components?

I'm thinking if they're used for the exact same thing on elements and components, I would imagine they should be named the same thing.

The reason I'm asking is because I can create the rule to do what you're saying, but I would prefer that the metal API was more consistent in this regard.

What do you think?

@bryceosterhaus
Copy link
Member Author

bryceosterhaus commented Jun 16, 2016

The difference is that a ref is stored in memory of the component

<ParentComponent>
      <Child ref="childComponent" />
</ParentComponent>

ParentComponent can now access the child via this.components.childComponent

However, a static html element such as a <div /> can not be given a ref. A key is given specifically for when there are multiple conditionally rendered items such as a list.

Refs are also used to track the internal components when they are conditionally rendered with different state.

<ParentComponent>
      <Child name="foo" ref="child1" />

      {someBool &&
             <Child name="bar" ref="child2" />
      }

      <Child name="baz" ref="child3" />
</ParentComponent>

When using refs keep track of component state, whereas a key is just for ordering. When adding a ref to a component, a key is internally provided as this.key = this.ref. This way we never need to add both key and ref.

I think the overall purpose of a ref is to identify some internal state to a given component and the purpose of key is for ordering of conditionally rendered elements.

Let me know if that makes a little more sense.

@bryceosterhaus
Copy link
Member Author

After writing this issue out and going back to tinker with keys and refs a bit. I am sort of in the same boat as you, kind of confused on the API. So I created an issue to discuss it on metal. metal/metal.js#129

We can figure out the formatting issue after that discussion is fleshed out.

@natecavanaugh
Copy link
Contributor

Heya Bryce,
Since this one has been open for a while and things seem to have changed on the metal side, I'm going to close it out for now.

Please reopen or open a new one if we still need to work out the details of this one.

Thanks Bryce,

@AngeloYoun
Copy link

AngeloYoun commented Nov 1, 2016

Hey Nate,

Metal doesn't require keys or refs for components in an array. It's "supposed" (there were some bugs where it wasn't that are now fixed) to handle that case behind the scenes.

Source formatter shouldn't be flagging 'Missing "key" prop for element in array" when using metal.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants