Array items now have unique, stable keys (#1046)

[fsteger]

Reasons for making this change

Array items did not have stable, unique keys and instead had to rely on the index of the item.
Fixes #1046
Fixes #1333

Checklist

• I'm updating documentation
  • I've checked the rendering of the Markdown text I've added
• I'm adding or updating code
  • I've added and/or updated tests
  • I've updated docs if needed
• I'm adding a new feature
  • I've updated the playground with an example use of the feature

[fsteger]

When using componentWillReceiveProps, the incoming props may be updated via onChange before the setState call completes. Latest change moves onChange to setState callback to ensure it's updated first.

This didn't appear to be an issue with getDerivedStateFromProps.

[epicfaace]

Looks good, can you add some tests?

[epicfaace]

I think it might be cleaner (and easier to migrate to React 16) just to use getDerivedStateFromProps and https://github.com/reactjs/react-lifecycles-compat for now?

[fsteger]

Done in latest commit.

[epicfaace]

@fsteger do you think there is any way to pass on this benefit to those who make their own ArrayFieldTemplates? As of now, they will only get this benefit if they use the default template.

[fsteger]

@fsteger do you think there is any way to pass on this benefit to those who make their own ArrayFieldTemplates? As of now, they will only get this benefit if they use the default template.

Yeah; that was my intention. Each item in the array of props.items in ArrayFieldTemplate should now have access to the key property. I had missed this initially for fixed arrays, but it should consistently be present now.

I updated the customArray.js in the playground to use the key, and updated the "should move from first to last in the list" test (ArrayField_test.js), which uses a custom ArrayFieldTemplate, to include the keys as well.

[epicfaace]

I don't think there's any point in exposing the key to the user as the id? Currently ids are only used as per the idSchema (for example, root_listOfStrings_0) so having another function for id may also not be the best.

[fsteger]

What would you recommend here?

[epicfaace]

Can we just remove the id?

Or is there a particular reason why you thought of adding it?

[fsteger]

I'm using the id right now to ensure that the key does not change inside the ArrayField tests. If you remove the id, the tests for inserting new rows will fail. The re-order and delete tests only pass when the attribute is removed because the id both before and after the action are undefined, but these should really test to ensure an id exists as well.

If we remove the id, I'm not sure how these tests should be updated to verify the correct behavior as the key doesn't appear to be exposed anywhere else.

Regarding the idSchema, were you thinking of just passing the key into the toIdSchema method for each item, and then accessing it via props.idSchema.key?

[epicfaace]

Got it. I don't think we should add it to the idSchema or expose it as the id attribute, though, given that the array item ids have no meaning apart from being unique and stable (as opposed to the other ids in the idSchema).

Here are two possible solutions:

• Can we expose the key as data-rjsf-itemkey or something instead?
• Or, we don't expose the key at all -- in our tests, create a custom ArrayFieldTemplate that exposes the key in the DOM so that the tests can access it.

[fsteger]

I removed the id attribute, and updated the tests to use a custom template.

[epicfaace]

Great, thanks!