1276206 - Reps: If a grip/object's property name is "more", it is clobbers the caption

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Lin Clark [:linclark]]

Because Reps use the key "more" for captions, they will collide with any properties named more.

For example, if you have the following object:

> `{a: "a", b: "b", more: "c", d: "d", e: "e"}`

The output would be:

> `{a: "a", b: "b", more: "c"}`

But it should be:

> `{a: "a", b: "b", more: "c", more...}`

Comment 1

[Lin Clark [:linclark]]

Attachment: Bug1276206.patch

Removing the key is fine in this case. React will use its default, which is the array index of the Caption element. The "more..." caption should never be at a different position in the array, so using index as the key should have the same effect.

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8757279 [details] [diff] [review]
Bug1276206.patch

Review of attachment 8757279 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch!

Honza

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Hm... I am now seeing:

"Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `Grip`. See https://fb.me/react-warning-keys for more information."


Honza

Comment 4

[Lin Clark [:linclark]]

Attachment: Bug1276206.patch

I realized that if we move the caption out of the array and just pass it in directly in the children arguments, as a sibling to props, then we won't get the warning.

Honza, what would you think of an approach like this? Not fully implemented yet, just wanted to get your feedback first.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Lin Clark [:linclark] from comment #4)
> Created attachment 8757319 [details] [diff] [review]
> Bug1276206.patch
> 
> I realized that if we move the caption out of the array and just pass it in
> directly in the children arguments, as a sibling to props, then we won't get
> the warning.
> 
> Honza, what would you think of an approach like this? Not fully implemented
> yet, just wanted to get your feedback first.
Interesting, I didn't know keys work that way.
Anyway, works for me.

Honza

Comment 6

[Hemanth Kumar Veeranki [:harry7]]

I would like to work on this. Can Someone help me where to start?

Comment 7

[Lin Clark [:linclark]]

I have a patch up for this that I just have to apply a few changes to. Sorry that wasn't clearer, I didn't realize that my assignment to myself had been removed.

Another issue that would probably be good is this one: https://bugzilla.mozilla.org/show_bug.cgi?id=1264686

Comment 8

[Hemanth Kumar Veeranki [:harry7]]

(In reply to Lin Clark [:linclark] from comment #7)
> I have a patch up for this that I just have to apply a few changes to. Sorry
> that wasn't clearer, I didn't realize that my assignment to myself had been
> removed.
> 
> Another issue that would probably be good is this one:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1264686
That depends on some other bug can you give me a good bug similar but free to take?

Comment 9

[Lin Clark [:linclark]]

Honza, if you're working on this be advised there's an easier way to handle this.

What you can do is use the rest syntax, e.g. ...values, to turn from an array to individual children. Then you don't need keys. As long as we aren't reordering any children, I think this is the best solution

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1276206.patch

(In reply to Lin Clark [:linclark] from comment #9)
> Honza, if you're working on this be advised there's an easier way to handle
> this.
> 
> What you can do is use the rest syntax, e.g. ...values, to turn from an
> array to individual children. Then you don't need keys. As long as we aren't
> reordering any children, I think this is the best solution
Nice, I've done it that way.

- Object rep fixed + test updated.
- Grip rep fixed + test updated.
- Array/GripArray also changed, but just to make the code a bit more consistent. These reps don't have the problem (that's way there are no tests for these two).

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b96ea37cd4b

Honza

Comment 11

[Lin Clark [:linclark]]

Comment on attachment 8795312 [details] [diff] [review]
bug1276206.patch

Review of attachment 8795312 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks!

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks Lin!

Honza

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/327bd3387ff6
Avoid caption clobber if more prop exists. r=linclark

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/327bd3387ff6