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

Array item keys should not be based on array index #1046

Closed
2 tasks done
djurre opened this issue Oct 11, 2018 · 9 comments · Fixed by #1335
Closed
2 tasks done

Array item keys should not be based on array index #1046

djurre opened this issue Oct 11, 2018 · 9 comments · Fixed by #1335

Comments

@djurre
Copy link

djurre commented Oct 11, 2018

Prerequisites

  • I have read the documentation;
  • In the case of a bug report, I understand that providing a SSCCE example is tremendously useful to the maintainers.

Description

In arrays, the key is currently based on the array index, see https://github.com/mozilla-services/react-jsonschema-form/blob/master/src/components/fields/ArrayField.js#L49.

This is not recommended and can give rise to a number of issues, see https://reactjs.org/docs/lists-and-keys.html

In particular this:
image

Steps to Reproduce

Run https://jsfiddle.net/wt4hkpy2/, and try to remove 'item 1' or 'item 2'. The last item in the array gets removed.

This is because before removal there are 3 items with in index 0, 1 and 2. After removal there are two items with index 0 and 1. Thus the react reconciler infers that the item with index 2 must have been removed.

(I realise that by making the input controlled in the above example the problem disappears, but I purposely used the 'props to state' antipattern as it is the smallest way I could think of to demonstrate the key issue)

Expected behavior

The correct array item is removed

Actual behavior

The last array item is removed

Version

1.0.2, but problem exists in current master as well

@Muntaner
Copy link

I think I hit this issue in my application and I reported the problem here:
#1079

You mention in your issue "(I realise that by making the input controlled in the above example the problem disappears, but I purposely used the 'props to state' antipattern as it is the smallest way I could think of to demonstrate the key issue)"

Can you please clarify what you mean?

@djurre
Copy link
Author

djurre commented Nov 19, 2018

I meant making the value of the input based on the props instead of the internal component state. See this fiddle: https://jsfiddle.net/esgybn75/

Which of course is fine for simple widgets, but for some more advanced custom widgets where you need a state it will break.

@glasserc
Copy link
Contributor

Yes, that sounds like a problem. Unfortunately I'm not really sure what other key we have available to us that we could possibly use. Do you have a suggestion?

@christianeide
Copy link

christianeide commented Jan 12, 2019

I have currently the same problem with an array that I want to sort by https://github.com/atlassian/react-beautiful-dnd. Currently the arrayitems dont get a permanent ID when they are created, so I have not found any data that could be used as a safe key. I have looked through all available data the template is receiving without finding any unique permanent data for an arrayitem. An arrayfieldtemplate is passed down an ID, but the ID is changing based on the elements index in the array.

Would it be possible to add a unique id to each item in an array when they are added to the index? Thats not part of formData?

@epicfaace
Copy link
Member

@christianeide perhaps. Do you have a method in mind to do this unique id assignment -- able to make a PR fixing this?

@epicfaace
Copy link
Member

Perhaps using something like shortid as outlined in this article?

@f0rr0
Copy link

f0rr0 commented Apr 30, 2019

any updates on this? anything could be used to generate a unique id but it needs to be persisted outside of formState. I have been unable to figure out where that should be on reading the code. it seems like formState is the only source of truth for rendering an array

@kylehalleman
Copy link

Like @f0rr0, would be happy to submit a PR if direction is given on what's the best approach and where we'd make this update.

@epicfaace
Copy link
Member

@kylehalleman the best approach would be to create a unique id for each array item as it is added (or rendered for the first time), and set that to the key. You could use a library like shortid to do so; see https://medium.com/@robinpokorny/index-as-a-key-is-an-anti-pattern-e0349aece318

fsteger pushed a commit to fsteger/react-jsonschema-form that referenced this issue Jun 25, 2019
epicfaace pushed a commit that referenced this issue Jul 9, 2019
* Array items now have unique, stable keys (#1046)

* update package-lock.json

* update package-lock.json

* fix: ensure state has been updated before calling onChange

* Add tests for array item keys. Include item key for fixed item arrays.

* Update ArrayField to use getDerivedStateFromProps via polyfill

* fix: remove id; use custom array template for tests.

* fix: use custom arraytemplate for key test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants