refine invariant error message at scrollToIndex

[sasurau4]

Summary

I refined the error message of scrollToIndex.

When I used scrollToIndex with index:0 and data that length is 0, I met the odd error message Invariant Violation scrollToIndex out of range: requested index 0 but maximum is -1.

Next, I thought that scrollToIndex with index:-1 meant scrollToTop without checking data length. I met Invariant Violation: scrollToIndex out of range: requested index -1 but maximum is -1

Finally, I wondered what will happen to use scrollToIndex with index:-1 and data that length is 5. The result is Invariant Violation: scrollToIndex out of range: requested index -1 but maximum is 5

The above error messages will confuse us. I clarified the boudaries and separated the error messages

Changelog

[General] [Fixed] - Clarified the boundaries in error message of scrollToIndex

Test Plan

I added 3 test cases to cover the new error messages for VirtualizedList.
Run yarn test and confirm it passes.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	10,903,792	243,728

Base commit: 90f60a8

[elicwhite]

Your test plan says not needed, tests must pass, does that mean there are tests for this? If so, since you are adding error messages and trusting tests, I'd expect to see additional tests covering this new behavior.

[sasurau4]

@TheSavior Thanks for pointing out 👍 I added test cases.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	6,758,892	-599
android	hermes	armeabi-v7a	6,405,934	-602
android	hermes	x86	7,140,312	-578
android	hermes	x86_64	7,031,618	-603
android	jsc	arm64-v8a	8,927,824	-217
android	jsc	armeabi-v7a	8,567,266	-215
android	jsc	x86	8,752,361	-224
android	jsc	x86_64	9,329,309	-210

Base commit: 90f60a8

[elicwhite]

This one I’m not sure about. With VirtualizedList you can do tail loading. So when you scroll to the bottom of the list you can fetch more data and render it. If scrollToIndex is greater than the data length I’d expect fail loading to be possible and for it to continue trying to scroll to the index. I’m not sure what the current behavior is but I’m a bit concerned about throwing in this case.

Can you either remove this condition or do a bit more investigation into that edge case?

[sasurau4]

I remove this condition.

[elicwhite]

I'm so sorry. I missed that the original code had invariant(index < getItemCount(data). I didn't see that until now. The original condition I asked you to remove seems like it was consistent with the original behavior. Can you add it back? 😓

[sasurau4]

I revert the f898b30 👍

[facebook-github-bot]

@TheSavior has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @sasurau4 in 78d2b3c.

^{When will my fix make it into a release? | Upcoming Releases}