Animation bug

[fortmarek]

Description

Waiting for a release with this PR.

This PR adds support for layout animations - however, they still don't work flawlessly:

• on both platforms, a cell can appear to first fly across the screen before getting into its position - this happens when a new cells appears on the screen, but a cell from the recycler pool that happens to be on the opposite side is used to fill that space
• on Android, there are is an error re: removeView. This originates outside of our stack and it is quite plausible the experimental nature of react-native's LayoutAnimations is at fault here

This means the animation added to List is currently not flawless - however, I'd still keep it and we can iterate on the example as we try to fix it.

Reviewers’ hat-rack 🎩

Try out the list example - the only bug in the animation should be the "flying" cells on iOS.

Screenshots or videos (if needed)

Checklist

• I have added a changelog entry following the Keep a Changelog guidelines.

[naqvitalha]

@fortmarek I really like the approach and that we can do this without having to rely on RN APIs. I wonder if this works with reanimated. Also, you were right and things do work fine on Android without any changes but it makes me worry. Why is draw being bypassed and can a future change totally break this? Thoughts?

[fortmarek]

I wonder if this works with reanimated.

Yes, I wonder, too. I am waiting for this to be merged and I will try it out.

Why is draw being bypassed and can a future change totally break this?

Is the draw being bypassed? I think only the whole fixLayout function is not executed or am I wrong?

[naqvitalha]

I was referring to Android. Layout animations are working fine without any special handling. Not sure how.

[fortmarek]

I was referring to Android. Layout animations are working fine without any special handling. Not sure how.

Ahh, right - neither do I know. We might need to investigate if we see some additional issues with layout animations.

[fortmarek]

@naqvitalha @ElviraBurchik this should be now ready for review.

[ElviraBurchik]

I get this error while swiping left/right in List example on iOS

ERROR  TypeError: _list$current.prepareForLayoutAnimationRender is not a function. 
(In '_list$current.prepareForLayoutAnimationRender()', 
'_list$current.prepareForLayoutAnimationRender' is undefined)

[ElviraBurchik]

I'd love to see an example of how it should be used in the code if I was a developer using the library. Can we add an example from below?

[fortmarek]

I think we are missing examples in general. But I worry adding examples for individual props will make this page too long as for each example you need extra code, so I would not add it as of now.

[ElviraBurchik]

Well, we can start adding examples and divide this page into subpages when it becomes too long. Otherwise, we'll just forget, why not to add now?

[fortmarek]

I will flag it in the issue we have but the example should cover more ground than this prop only which is why I think it's out of scope for this (we'll need to look at it from more broader perspective than this prop)

[ElviraBurchik]

Why do we need this index now?

[fortmarek]

we have not had stable ids before and you need index for the keyExtractor method.

[naqvitalha]

@fortmarek This check looks overly complicated to me. Can you simplify it? Also, I'd recommend always using === in favour of == it just makes things easier to understand. I was thinking that this would clearly fail when keyExtractor is undefined till I realized you were using two equals :|

Also, I'd recommend not passing this method at all if there's no key extractor. The default implementation now passes indexes as stableIds and indexes aren't really stable :) I think it doesn't match RLVs expectations

[fortmarek]

Also, I'd recommend always using === in favour of == it just makes things easier to understand.

Generally, I agree but in case when I want to check whether something is null | undefined, this is the easiest way and I believe it's pretty common.

I was thinking that this would clearly fail when keyExtractor is undefined till I realized you were using two equals :|

Typescript would complain if there was a possibility of keyExtractor being null or undefined

Also, I'd recommend not passing this method at all if there's no key extractor.

This might be more complicated than it sounds. I suppose we don't want to create a new DataProvider but it's also not possible to change getStableId to null (which is necessary in case user adds a keyExtractor prop and then removes it - unlikely, but technically possible). The simplest solution would be to access the private property of _hasStableId and change that - but I cannot because that's in recyclerlistview. Additionally, this property is only set in constructor of the DataProvider. Let me know if it's ok to recreate the dataProvider when the getStableId function is changed @naqvitalha. Or we can change the _hasStableIds to be a computed property based on getStableId being null and change the type of the getStableId property to be (index: number) => string | null.

The default implementation now passes indexes as stableIds and indexes aren't really stable :)

This is the same implementation is in recyclerlistview (see here) with the difference of _hasStableIds value (which I cannot directly change as mentioned above)

[fortmarek]

This check looks overly complicated to me. Can you simplify it?

I can remove the first flashList.props == null check but otherwise, I don't find the check "overly" complicated.

[naqvitalha]

Is this O(n) or O(1) ?

[fortmarek]

isEmpty is O(1)

[fortmarek]

plus animationKeys will never be a long array anyway

[naqvitalha]

Hey I was actually asking about animationKeys :P method. Does it go through children or something like that? isEmpty should be O(1) like you said.