Sort order gets messed up when coupled to a redux flow

[nervetattoo]

First, thanks for the work on this library. React-draggable-list does all we need, except for the fact that we don't really get it to work alongside redux and redux-form.

The problem lies in the fact that react-draggable-list actually reorders the incoming list in componentWillReceiveProps when it's re-rendered after a drag has ended. What we do is something like:

<DraggableList
  ...
  list={listFromReduxStore}
  onMoveEnd={(list,item,oldIndex, newIndex) => {
    dispatchReduxResort('mylist', oldIndex, newIndex)
}} />

This dispatch will in turn cause the redux store to build a new listFromReduxStore that is correctly sorted. When this list is passed to react-draggable-list it will get resorted in componentWillReceiveProps before the UI re-renders in a faulty state because the items were moved around based on their indexes, twice.

We're happy to contribute to a fix, but I'm not sure how you'd like to approach it. I haven't dived deeply into the source, but maybe moving the splicing out from componentWillReceiveProps and into the move end handler is the way to go?

[Macil]

I'm not sure I understand the issue. Doesn't componentWillReceiveProps only change the list if it's during a drag and the currently-dragged item is moved in the list? How does that reordering break something? Can you build a small example (based on example/Example.js if that makes it easiest) that shows the issue?

[nervetattoo]

I will see if I can create a small example tomorrow!

[nervetattoo]

Alright. I've built this simple example that shows the error. And here's the source code

[Macil]

Oh, so you're using the oldIndex and newIndex parameters to onMoveEnd and ignoring the newList parameter. The oldIndex/newIndex parameters have been pretty neglected and apparently don't accurately describe the move. We'll fix that, which should fix your example.

At the current time, things will work if you ignore the oldIndex/newIndex parameters and use the newList argument, though I can see how it doesn't make for as nice of a redux action.

EDIT: Looking closer I see it doesn't have to do with this.

[nervetattoo]

It is some sort of timing issue. The component will first be re-rendered due to redux passing in a new list to react-draggable-list, and then componentWillReceiveProps excepts to get the last version of the list, and then it reorders itself, leaving the local state of the draggable-list out of sync with the redux state. If you move https://github.com/StreakYC/react-draggable-list/blob/master/src/index.js#L112 to happen before calling onMoveEnd it will work because it will first reorder locally and then get the new state from redux and subsequentially re-render.

I can think of a few solutions on top of my head:

• Allow disabling componentWillReceiveProps
• Use the return value of onMoveEnd to dictate the new state

One thing we could do, ofc, is to extend the class and override componentWillReceiveProps.

[Macil]

I figured it out. The issue is here: https://github.com/nervetattoo/react-draggable-list-bug-redux/blob/0c6dbcf34e2f7bdc7b35c278b20b1b2b40360870/src/App.js#L41. You're setting the key to the item's current index. React-draggable-list (well, and React itself) identify the item by the key. When you move an item up the list, you're changing the keys of many items in the list, including the item that was dragged (which happens to trip react-draggable-list's special handling of the currently-dragged item being moved in the list). If you change the line to this, then it works, and also React knows it can move the elements instead of updating all of the elements to have new names:

    const planets = this.props.planets.map(name => ({name,key: name}))

[nervetattoo]

That fixes the contrived example; However in our real codebase we already did this, with no luck. We do have some other components in the hierarchy that could be related to the issue, so I will see if I can expand upon my reproduction so it fails with proper keys set as well. The real code is a complex, sortable form using redux-form, and nested react-draggable-list components, so theres more nuance to it ...

[Macil]

Unless there's some specific use-case I don't know about, I'm currently leaning toward accepting that moving (or changing the key of; same thing to react and react-draggable-list) the dragged item in response to onMoveEnd results in undefined behavior, but anything else causing trouble here would be a good reason to investigate further. Let me know if you can reproduce the issue with proper keying.

I've got some ideas in mind that could get rid of the need for the copy of list kept in state and componentWillReceiveProps's occasional reordering of the list that could solve non-keying-related issues if any exist, but I think it would be best for me to develop and test against the problematic cases if they do exist.

[nervetattoo]

Alright. I've reproduced the bug with proper keys. The issue is apparently not caused solely by including redux, it also required redux-form as a middleman. I'll concede that it might not be the most important use case, but redux-form is fairly popular and it would be nice if react-draggable-list played well with it.

Sadly the example is now less clear because redux-form introduces some new levels of indirection that isn't entirely intuitive.

I still find it a bit weird to have internal state reordering happening in componentWillReceiveProps, instead of purely as part of an event handler. I have a strong feeling that this is caused by the fact that we have multiple asynchronous calls (setState in react-draggable-list, and internally in redux-form + redux) without a guaranteed execution order.

Let me know if you want help from me in coming up with a fix, although I feel that you understand the internals to react-draggable-list far better.

[nervetattoo]

Do you have any intuitive ideas about how we can forward in fixing this? We need to find a way to move forward, and I'd be happy to take a stab at a PR, but in that case it would be nice with some pointers in the direction you think it should go.

[skolmer]

I use this library in combination with redux-form and it's working great.

The problem here is that the key of your list item component has to be the same even if the index of the item changes.

In your example posted above you are using the (reordered) index from the current redux form state (fields) to select a key from initialValues. Instead you should use formValueSelector to get the current state of your (reordered) list from the redux-form state and not use the list provided via initialValues. For more details please see the following issue https://github.com/erikras/redux-form/issues/1727#issue-175556288

[abrad45]

I've come across an interesting case that seems related to this. React-Draggable-List + Redux-Form FieldArrays + Redux-Form Field Level Validation = 😢

I've been working with a component, let's call it Grandparent that looks like this...

// Decorator: adds indices to each object in the FieldArray
// since DraggableList doesn't include them
const getListOfFieldsets = () => {
    return fields.getAll() && fields.getAll().length ?
        fields.getAll().map((fieldset, index) => Object.assign(fieldset, {index: index})) :
        [];
};

const onMoveEnd = (newList, movedStash, oldIndex, newIndex) => {
    fields.move(oldIndex, newIndex);
};

...

<DraggableList
    list={getListOfFieldsets()}
    onMoveEnd={onMoveEnd}
    template={Parent}
    />

where Parent had eight fields in it, as well as a nested component (Child) that contained two more.

const ComponentWithFields = () => {
    return (
        <Field />
        <Field />
        <Field />
        ...
        <Child />
    );
};

Each of the fields in Parent and Child have Redux-form's Field Level Validation which marks them required (validate={value => value ? undefined : 'Required'}).

---

When you'd reorder two Fieldsets, the redux-form field-level validation stopped working for most of these fields, but not for the ones in the nested component Child. This would enable you to submit a field with empty fields that were marked required. I fixed this problem by adding a component between Grandparent and Parent so that the component you set in the template prop of DraggableList has no redux-form fields in it.

This seems a little hacky, but it does work. Still, it seems indicative of the fact that there's something screwy going on in one of these plugins.

[Macil]

Your issue appears to be that you're not using the itemKey prop, similar to the previous issue. Is React not giving you a warning about the missing required prop?

[abrad45]

@AgentME thanks for the quick reply.

I am using the itemKey prop. I won't modify the previous example for posterity's sake, but to be more thorough, my component looks like this. fieldset.id is a uuid generated by https://www.npmjs.com/package/uuid

const onDeleteStash = (index) => {
    fields.remove(index);
};

// Decorator: adds indices to each object in the FieldArray
// since DraggableList doesn't include them
const getListOfFieldsets = () => {
    return fields.getAll() && fields.getAll().length ?
        fields.getAll().map((fieldset, index) => Object.assign(fieldset, {index: index})) :
        [];
};

const onMoveEnd = (newList, movedStash, oldIndex, newIndex) => {
    fields.move(oldIndex, newIndex);
};

...

<DraggableList
    list={getListOfFieldsets()}
    itemKey="id"
    template={Parent}
    onMoveEnd={onMoveEnd}
    unsetZIndex
    commonProps={{
        onDeleteStash: onDeleteStash,
    }}
    />

[Macil]

Can you post a full example project that I could run that reproduces this issue? I haven't been able to reproduce this issue with proper itemKey usage.

[abrad45]

@AgentME this may be a little difficult, but I'll see if I can pull this off sometime soon.

[abrad45]

I set out to find a fix for this issue this morning and... I couldn't do it. I'm not sure why. The change I made? I set itemKey to the index I add in getListOfFieldsets().

// Decorator: adds indices to each object in the FieldArray
// since DraggableList doesn't include them
const getListOfFieldsets = () => {
    return fields.getAll() && fields.getAll().length ?
        fields.getAll().map((fieldset, index) => Object.assign(fieldset, {index: index})) :
        [];
};
...

<DraggableList
    list={getListOfFieldsets()}
    itemKey="index"
    template={Parent}
    onMoveEnd={onMoveEnd}
    unsetZIndex
    commonProps={{
        onDeleteStash: onDeleteStash,
    }}
    />

I'm not entirely sure why this changes anything in a substantial way. If I understand what's going on correctly, index changes each time DraggableList is re-rendered because getListOfFieldsets() runs and rewrites each index value, whereas id is the uuid I was mentioning before and doesn't change. I'd expect index would be a worse key than id but it appears to have resolved many of the issues I was seeing.

@AgentME does this behavior change make sense to you?

[Macil]

Having itemKey set to index should cause the problem you mentioned earlier (it's exactly what caused this issue for someone above). If that's working now with itemKey=index, then I find that really surprising and more convincing that there is an issue in react-draggable-list. It's probably cancelling out the issue you ran into before, and will break again once that bug is fixed or if other changes are made to the project.

I still don't know what would cause this issue. I've used redux before, but I haven't found any reason why react-draggable-list would work differently with it, and I've failed at making my own reproduction cases. I would be able to investigate this more with a reproduction case.

[abrad45]

I'm glad we drew the same conclusions here. The issue is that the "reproducible case" is my project at work I cannot post code from, and the case that works is the demo I was able to post for you. By changing this value from id to index in my project at work, I've "fixed" (masked, as you propose) the problem. 🙃

[Macil]

I've just released v4.0 of react-draggable-list which I believe will solve this issue. I never managed to reproduce the issue, but I think I know what code was responsible, and I ripped that code out in the process of updating react-draggable-list to not use deprecated React features. DraggableList no longer keeps a separate copy of the list in state, which simplifies some things and should avoid this issue.