[1/2] Make FlatList permissive of ArrayLike data #36236
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D43465654 |
NickGerleman
changed the title
Base commit: a2f155f |
kraenhansen
reviewed
Feb 22, 2023
Libraries/Lists/FlatList.js
Outdated
| data && | ||
| (typeof data === 'object' || typeof data === 'function') && | ||
| typeof data.length === 'number' && | ||
| Symbol.iterator in data |
There was a problem hiding this comment.
Is this just to get some proof that it's array-like? I mean, neither the block below nor any other part of the component depends on the data being iterable.
There was a problem hiding this comment.
Yeah, Flow $ArrayLike requires iterability, where TypeScript ArrayLike does not, and I wanted to preemptively avoid the confusion the mismatch would cause and flag anything following the wrong ArrayLike.
But this actually ended up flagging that Flow $ArrayLike requiring iterability is more likely a mistake, and that requirement was added after-the-fact in a separate refactor. So I think it is likely this check will be removed.
Libraries/Lists/FlatList.js
Outdated
Comment on lines
525
to
528
| data && | ||
| (typeof data === 'object' || typeof data === 'function') && | ||
| typeof data.length === 'number' && | ||
| Symbol.iterator in data |
There was a problem hiding this comment.
I suggest moving this to _checkProps. It seem more like a check for a precondition of the prop than something that needs to be checked potentially multiple times per render: I mean, the call pattern is a discretion of the VirtualizedList component and we don't expect data to change except when the prop updates.
There was a problem hiding this comment.
The tricky part of that is for compatibility, we cannot trigger an invariant violation like other checks in _checkProps, and instead just want to tell the underlying VirtualizedList not to try to read from the source.
VirtualizedList does assume getItemCount is somewhat cheap, so I agree this checks are a little icky to be adding to be run repeatedly. But it avoids introducing state into the list. I think I should be able to make this check marginally cheaper though.
Summary: Pull Request resolved: facebook#36236 D38198351 (facebook@d574ea3) addedd a guard to FlatList, to no-op if passed `data` that was not an array. This broke functionality where Realm had documented using `Realm.Results` with FlatList. `Real.Results` is an array-like JSI object, but not actually an array, and fails any `Array.isArray()` checks. This change loosens the FlatList contract, to explicitly allow array-like non-array entities. The requirement align to Flow `ArrayLike`, which allows both arrays, and objects which provide a length and indexer. Flow `$ArrayLike` currently also requires an iterator, but this is seemingly a mistake in the type definition, and not enforced. Though `Realm.Results` has all the methods of TS `ReadonlyArray`, RN has generally assumes its array inputs will pass `Array.isArray()`. This includes any array props still being checked [via prop-types](https://github.com/facebook/prop-types/blob/044efd7a108556c7660f6b62092756666e39d74b/factoryWithTypeCheckers.js#L548). This change intentionally does not yet change the parameter type of `getItemLayout()`, which is already too loose (allowing mutable arrays). Changing this is a breaking change, that would be disruptive to backport, so we separate it into a different commit that will be landed as part of 0.72 (see next diff in the stack). Changelog: [General][Changed] - Make FlatList permissive of ArrayLike data Differential Revision: D43465654 fbshipit-source-id: 73acd565a51dd7dce693fcab9ed06b63284c97df
|
This pull request was exported from Phabricator. Differential Revision: D43465654 |
NickGerleman
force-pushed
the
export-D43465654
branch
from
648aa3a
to
c318f27
Compare
NickGerleman
added a commit
to NickGerleman/react-native
that referenced
this pull request
Feb 22, 2023
Summary: Pull Request resolved: facebook#36236 D38198351 (facebook@d574ea3) addedd a guard to FlatList, to no-op if passed `data` that was not an array. This broke functionality where Realm had documented using `Realm.Results` with FlatList. `Real.Results` is an array-like JSI object, but not actually an array, and fails any `Array.isArray()` checks. This change loosens the FlatList contract, to explicitly allow array-like non-array entities. The requirement align to Flow `$ArrayLike`, which allows both arrays, and objects which provide a length, indexer, and iterator. Though `Realm.Results` has all the methods of TS `ReadonlyArray`, RN has generally assumes its array inputs will pass `Array.isArray()`. This includes any array props still being checked [via prop-types](https://github.com/facebook/prop-types/blob/044efd7a108556c7660f6b62092756666e39d74b/factoryWithTypeCheckers.js#L548). This change intentionally does not yet change the parameter type of `getItemLayout()`, which is already too loose (allowing mutable arrays). Changing this is a breaking change, that would be disruptive to backport, so we separate it into a different commit that will be landed as part of 0.72 (see next diff in the stack). Changelog: [General][Changed] - Make FlatList permissive of ArrayLike data Differential Revision: https://www.internalfb.com/diff/D43465654?entry_point=27 fbshipit-source-id: 15c9e19414b82ea8676df0428c5540097107259c
| _getItemCount = (data: ?Array<ItemT>): number => { | ||
| if (Array.isArray(data)) { | ||
| _getItemCount = (data: ?$ArrayLike<ItemT>): number => { | ||
| if (data != null && typeof Object(data).length === 'number') { |
There was a problem hiding this comment.
If it's ArrayLike obj and not null, it must have a length prop defined. ?
And we don't want those with only size defined or do we ?
any specific reason for new Object instance overhead?
Suggested change
| if (data != null && typeof Object(data).length === 'number') { | |
| if (data != null && typeof data.length === 'number') { |
There was a problem hiding this comment.
Per MDN this should return the existing object if it is one, but the check would cause an exception if someone passed say, a number, and the point here has been to guard against invalid data gracefully for legacy reasons.
|
This pull request has been merged in c03de97. |
kelset
pushed a commit
that referenced
this pull request
Mar 6, 2023
Summary: Pull Request resolved: #36236 D38198351 (d574ea3) addedd a guard to FlatList, to no-op if passed `data` that was not an array. This broke functionality where Realm had documented using `Realm.Results` with FlatList. `Real.Results` is an array-like JSI object, but not actually an array, and fails any `Array.isArray()` checks. This change loosens the FlatList contract, to explicitly allow array-like non-array entities. The requirement align to Flow `ArrayLike`, which allows both arrays, and objects which provide a length and indexer. Flow `$ArrayLike` currently also requires an iterator, but this is seemingly a mistake in the type definition, and not enforced. Though `Realm.Results` has all the methods of TS `ReadonlyArray`, RN has generally assumes its array inputs will pass `Array.isArray()`. This includes any array props still being checked [via prop-types](https://github.com/facebook/prop-types/blob/044efd7a108556c7660f6b62092756666e39d74b/factoryWithTypeCheckers.js#L548). This change intentionally does not yet change the parameter type of `getItemLayout()`, which is already too loose (allowing mutable arrays). Changing this is a breaking change, that would be disruptive to backport, so we separate it into a different commit that will be landed as part of 0.72 (see next diff in the stack). Changelog: [General][Changed] - Make FlatList permissive of ArrayLike data Reviewed By: yungsters Differential Revision: D43465654 fbshipit-source-id: 3ed8c76c15da680560d7639b7cc43272f3e46ac3
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: Pull Request resolved: facebook#36236 D38198351 (facebook@d574ea3) addedd a guard to FlatList, to no-op if passed `data` that was not an array. This broke functionality where Realm had documented using `Realm.Results` with FlatList. `Real.Results` is an array-like JSI object, but not actually an array, and fails any `Array.isArray()` checks. This change loosens the FlatList contract, to explicitly allow array-like non-array entities. The requirement align to Flow `ArrayLike`, which allows both arrays, and objects which provide a length and indexer. Flow `$ArrayLike` currently also requires an iterator, but this is seemingly a mistake in the type definition, and not enforced. Though `Realm.Results` has all the methods of TS `ReadonlyArray`, RN has generally assumes its array inputs will pass `Array.isArray()`. This includes any array props still being checked [via prop-types](https://github.com/facebook/prop-types/blob/044efd7a108556c7660f6b62092756666e39d74b/factoryWithTypeCheckers.js#L548). This change intentionally does not yet change the parameter type of `getItemLayout()`, which is already too loose (allowing mutable arrays). Changing this is a breaking change, that would be disruptive to backport, so we separate it into a different commit that will be landed as part of 0.72 (see next diff in the stack). Changelog: [General][Changed] - Make FlatList permissive of ArrayLike data Reviewed By: yungsters Differential Revision: D43465654 fbshipit-source-id: 3ed8c76c15da680560d7639b7cc43272f3e46ac3
Merged
This was referenced Jan 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
D38198351 (d574ea3) addedd a guard to FlatList, to no-op if passed
datathat was not an array. This broke functionality where Realm had documented usingRealm.Resultswith FlatList.Real.Resultsis an array-like JSI object, but not actually an array, and fails anyArray.isArray()checks.This change loosens the FlatList contract, to explicitly allow array-like non-array entities. The requirement align to Flow
$ArrayLike, which allows both arrays, and objects which provide a length, indexer, and iterator.Though
Realm.Resultshas all the methods of TSReadonlyArray, RN has generally assumes its array inputs will passArray.isArray(). This includes any array props still being checked via prop-types.This change intentionally does not yet change the parameter type of
getItemLayout(), which is already too loose (allowing mutable arrays). Changing this is a breaking change, that would be disruptive to backport, so we separate it into a different commit that will be landed as part of 0.72 (see next PR).Changelog:
[General][Changed] - Make FlatList permissive of ArrayLike data
Differential Revision: D43465654