fix: fix getNativeScrollRef return type for FlatList#54735
fix: fix getNativeScrollRef return type for FlatList#54735janpe wants to merge 2 commits intofacebook:mainfrom
Conversation
|
Hi @janpe! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
@rshest I see you’ve previously reviewed changes on this file. Any chance you could have a look? Thanks! |
|
How about @cipolleschi ? Any change you might have time to take a look at these changes? |
|
@huntie could you maybe look at this small PR? I saw that you've contributed to the code for Lists before so I thought reaching out to you to see if this correction for the types could get merged at some point. |
| | React.ComponentRef<typeof ScrollViewComponent> | ||
| | null | ||
| | undefined; | ||
| getNativeScrollRef: () => HostInstance | null; |
There was a problem hiding this comment.
@janpe This will deviate from our auto-generated types in the Strict API.
Can we check:
- Is
React.ComponentRef<typeof ScrollViewComponent> vs React.ComponentRef<ScrollViewComponent>the fix? - Can we update the source code in Flow for FlatList to align?
react-native/packages/react-native/ReactNativeApi.d.ts
Lines 2335 to 2337 in 4b4ae62
| | React.ComponentRef<typeof ScrollViewComponent> | ||
| | null | ||
| | undefined; | ||
| getNativeScrollRef: () => HostInstance | null; |
There was a problem hiding this comment.
Extra note: This needs to be PublicScrollViewInstance, so that it has the extra imperative methods we want.
|
Hey @janpe, FYI I'm looking at solving a related wider issue in #56673 (fingers crossed, we merge While I'd ideally want us to solve this problem once (for both the current manual TS API and the incoming Strict Flow→TS API), #52203 is indeed precedence. 🚧 Changes needed to land
|
e28a220 to
ea01eae
Compare
Much appreciated! See my changes and let me know if you'd like to request anything further. |
huntie
left a comment
There was a problem hiding this comment.
Let's make sure the TS changes pass CI validation!
|
|
||
| import * as ReactNativeFeatureFlags from '../../src/private/featureflags/ReactNativeFeatureFlags'; | ||
| import {type ScrollResponderType} from '../Components/ScrollView/ScrollView'; | ||
| import { |
There was a problem hiding this comment.
Can I ask that we omit changes outside the .d.ts files in this PR? I'm actioning these in #56718 🙂
There was a problem hiding this comment.
Oh, I must have misunderstood. Sorry for that! I've removed the changes to this file from the commit.
Summary: Pull Request resolved: facebook#56718 Update the return type of `getNativeScrollRef` on `ScrollView`, `FlatList`, `SectionList` to be `PublicScrollViewInstance` (previously, a mix of `HostInstance` and `React.ElementRef<>` types which did not include the imperative methods of `ScrollView`). - This type extends `HostInstance & ScrollViewImperativeMethods`, and is what the `ScrollView` ref chain already returns at runtime. - The previous types were either too broad (`HostInstance`), wrong (union with `View`), or required `$FlowFixMe` suppressions. Also removes `ScrollViewNativeComponent` from the public API surface, since it is an internal implementation detail not intended for external use (equivalent props are on the pre-existing `ScrollViewBaseProps` type). Related to: - facebook#52203 - facebook#54735 Changelog: [General][Fixed] - **Strict TypeScript API**: Update `getNativeScrollRef` return type across ScrollView, FlatList, and SectionList Differential Revision: D104223704
ea01eae to
70ca864
Compare
|
Warning JavaScript API change detected This PR commits an update to
This change was flagged as: |
Summary:
The Problem
When trying to measure the location of a View within a FlatList (ie. for scrolling to the view), the current recommended method is to use measureLayout on the nested view to determine its location inside the containing FlatList:
However the types for
FlatListgetNativeScrollRefdon't allow this.The solution
This solution is basically identical to that in #52203. The return value for
getNativeScrollRefshould beHostInstance | nullChangelog:[GENERAL] [FIXED] - Change FlatList.getNativeScrollRef return type definition to allow accessing the underlying HostInstance.
Test Plan:
None needed. This is only a type update exposing existing functionality.