Crash when data length changes "undefined is not an object (evaluating shift.targetValue)"

[debianw]

I have a DraxList with some items. When I remove one of the items and then add a new one and then I long press to drag an item then a crash happens.

Version: 0.8.0 RN: 0.63.3

[lafiosca]

@debianw Is it possible that you are changing the data of the item list array in-place without replacing the array itself?

[debianw]

@lafiosca . I just prepared a pretty basic TODO list App with Expo so you can reproduce it easily.

Here's the repo: https://github.com/debianw/rn-drag-todo-list This is a basic Expo App, so you can clone it and run it in your local. I created a quick api in Heroku with a basic CRUD, since this issue is only reproducible with real requests ( The issue is not happening if you use local state for testing, should be real state coming from server, I just wanted to create a more real scenario where you push and receive update from the server and handle optimistic updates in the client).

In case you need the server as well I created a repo (https://github.com/debianw/inmemory-todo-api). It is running in Heroku so you don't need to run it in your local (https://memory-todo-api.herokuapp.com/)

I'm guessing measures and shifts in DraxList are getting out of sync after several renders when itemCount changes.

Check the video and see how I can reproduce the issue, so you can follow the same steps.

Steps:

1. Create 4 tasks
2. Delete one task
3. Try to reorder an item by dragging it to a new position

https://user-images.githubusercontent.com/1002407/127780671-af95b1a1-5562-4fd0-b139-ffa8b9ed8a00.mov

[lafiosca]

Thank you, the sample project helped considerably. I was able to reproduce the bad behavior. This seems to be caused by a race condition related to the view registration/measurements that I have not run into previously. I am unsure how long this bug has existed, as I have not been working with lists of dynamic size since the feature was first implemented. It is also possible that something about the event order is different when using this in Expo versus a vanilla project, but I don't have the time right now to re-reproduce it in a new project.

I am going to publish a quick and dirty fix for this problem. It does not address the underlying race conditions, but it should prevent this fatal bug and be "good enough" for your uses, I hope.

[debianw]

@lafiosca Thanks, just for you to know this is happening in vanilla projects as well. In fact I'm working in a production App with vanilla React Native and that's where I catched the bug. I created this Expo demo App just for you :).

If you only want to avoid the crash but not fix the race conditions then unexpected behaviors could happen when ordering the items in the list.

[lafiosca]

When I say there's a race condition, it may not be as bad as it sounds... DraxList keeps several refs to arrays of item data: registrations, item measurements, and shift offsets. It tries to keep the lengths of these ref arrays in sync with the length of the data array. Whenever the itemCount changes, it updates the lengths of those arrays, either removing items from the end or extending them. The registration and item measurement lists may hold undefined values, so those are used when extending the length; but the shift list is all objects which need to exist when they are accessed. However, all three of these arrays were assumed to always stay the same length as each other, so the shrink/expand logic was based on checking only the item measurements array length.

There is also a state array of originalIndexes used for tracking reordered items until the data prop is updated; at that point, it resets to a list of the integers 0..N-1 (where N is the length of the data array).

The race condition I'm seeing with your project, which may have been introduced in one of the React Native version upgrades since the time this library was first created, is that in at least some cases the registrations/measurements are coming in before the ref array lengths have been extended. Even though those slots have not yet been allocated in the list, they are getting populated. Then when the useEffect executes to extend the lengths of the arrays, the item measurements array length is already the same as the itemCount because the measurements have populated early, so the shifts array length is not increased to match. Then when a drag begins, the (N-1)th item has no shift object, and the app crashes when trying to access a field on it.

Additionally, the current order of the effect triggers has also slightly broken the way the originalIndexes array is used when setting up for those registration/measurement handlers. It's possible for us to build the renderItem callback for an item which does not yet have an originalIndexes mapping, which means it will try to update itemMeasurementsRef.current[undefined] to add a measurement, which might also mess up the length of that array. I've updated the renderItem callback in this release to check before for undefined before using originalIndex. If that occurs, then it means the originalIndexes list has not yet been reset by its own useEffect, and after it does, the renderItem callback will be recreated as well with the proper index.

This may all sound rather arcane, and it is, but I am leaving it here more for a historical record than any other reason. Folks are welcome to try to dig into the code and improve it as I have not had the availability to do so in depth for a while. I would honestly love to go through and rewrite a new major version if I did; I have several thoughts on how I might change the approaches used. That may not be in the cards for a while, but I do try to make quick fixes for obvious or fatal bugs like this where I am able.

[lafiosca]

All that said, if you DO run into new unexpected behaviors from this, please feel free to open an issue!! I don't think the change I made should introduce any NEW bugs though. If you look at the diff, it's a pretty simple logic tweak that just makes execution the tiniest bit less optimized.

[lafiosca]

I should have also mentioned: the fix is in the latest release, 0.8.1

[debianw]

Working really good so far in new release 0.8.1