Fix finding props with testId

[fortmarek]

Description

Shop has had issues with triggering onViewableItemsChanged when getting the FlashList instance via getByTestId from @testing-library/react-native. Triggering flatList.props.onViewableItemsChanged returned undefined here.

The reason for this is that getByTestId does not get you FlashList component directly but whatever it renders as the last component. In our case, it's ScrollComponent (see here).

One possible fix (present in this PR) is to remove onViewableItemsChanged (which was unnecessary) from destructured props and pass it to ProgressiveListView which will in turn pass it to ScrollComponent. I have checked the failing test and now it correctly triggers the method.

Reviewers’ hat-rack 🎩

• [ ]

Screenshots or videos (if needed)

Checklist

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

[naqvitalha]

@fortmarek Does this mean that everything needs to be passed to ScrollComponent? It may not be feasible in cases where we're reimplementing an existing ScrollView prop.

Is it possible to suggest an alternative instead? Can we change the query?

[fortmarek]

Can we change the query?

The current behaviour is the expected one, so I don't think we can change the query. This test case inside the react-native-testing-library showcases that only native components are supposed to be returned - which corresponds to my findings where RCTScrollView is returned and FlashList not.

Is it possible to suggest an alternative instead?

I am actually not sure if react-native-testing-library is the right tool for the job here. It's supposed to be used for e2e tests and mimic user behaviour - so of course, you should be able to only interact with native components. If Shop used for those kinds of tests @quilted/react-testing, they could leverage the find method which finds a React element.

I don't like the solution proposed in this PR - although it fixes the issue, we should not have to pass all the props of FlashList to the native view. Instead, it seems to me the test is not written correctly.

@linddominic, what do you think about this?

[linddominic]

Can we change the query?

The current behaviour is the expected one, so I don't think we can change the query. This test case inside the react-native-testing-library showcases that only native components are supposed to be returned - which corresponds to my findings where RCTScrollView is returned and FlashList not.

Is it possible to suggest an alternative instead?

I am actually not sure if react-native-testing-library is the right tool for the job here. It's supposed to be used for e2e tests and mimic user behaviour - so of course, you should be able to only interact with native components. If Shop used for those kinds of tests @quilted/react-testing, they could leverage the find method which finds a React element.

I don't like the solution proposed in this PR - although it fixes the issue, we should not have to pass all the props of FlashList to the native view. Instead, it seems to me the test is not written correctly.

@linddominic, what do you think about this?

Hm. I don't know if the statement that react-native-testing-library is supposed to be used for e2e and that switching is the full solution.

I could dig into if there is another way to write the same test too see if that particular test in Shop is doing something iffy.

On a higher-level discussion if we want FlashList to be a drop-in replacement to FlashList I feel like this PR should get merged.

[fortmarek]

On a higher-level discussion if we want FlashList to be a drop-in replacement to FlashList I feel like this PR should get merged.

This is a good point. We should probably get this merged as FlatList passes all of its props to ScrollView (see here). I will update the PR to also pass other relevant props to ScrollView that are currently excluded.

[naqvitalha]

This is a good point. We should probably get this merged as FlatList passes all of its props to ScrollView (see here). I will update the PR to also pass other relevant props to ScrollView that are currently excluded.

Is that really a good idea? If you pass all the props things will break for e.g, stickyHeaderIndices. Most of them are deliberate omissions and the remaining ones are already forwarded to ScrollView.

I don't have an issue with just passing onViewableItemsChanged forward but I think this test should be changed anyway. It's okay to make assumptions on implementation details or use private APIs but then such tests need to be updated when things change :)

FlatList can stop forwarding this prop too and I wouldn't call it a breaking change even if some people need to fix their tests. It's going to be extremely difficult to maintain FlashList if we can't change internals when we want to. Honestly, I'd only want to worry about keeping public APIs same.

[fortmarek]

I don't have an issue with just passing onViewableItemsChanged forward but I think this test should be changed anyway. It's okay to make assumptions on implementation details or use private APIs but then such tests need to be updated when things change :)

I generally agree but at least initially, people will expect FlashList to mostly behave as FlatList does and if we can make a small change to achieve that without any real drawbacks, I think we should go for it. But let's keep the scope limited and just add onViewableItemsChanged which had no good reason to be excluded 👍

FlatList can stop forwarding this prop too and I wouldn't call it a breaking change even if some people need to fix their tests. It's going to be extremely difficult to maintain FlashList if we can't change internals when we want to. Honestly, I'd only want to worry about keeping public APIs same.

I think nobody is advocating for matching the internals completely or treating internals as a public API. We both agree that the way the test is written relies on the internal implementation which you never should do. But as mentioned, in this case we can accommodate Shop team as it should not really impact us in a negative way.

[ElviraBurchik]

Based on the discussion regarding this change, I agree that it makes sense to merge it

[naqvitalha]

Looks good, including this has no side effects.

[fortmarek]

@linddominic this has now been released in the 0.5.0 version 🚀