fix(switch): forward ref to the root view - #5065
Conversation
MikitasK
left a comment
There was a problem hiding this comment.
look good overall 👌
just 2 non-blocking suggestions to consider before merge:
| * Accessibility label for the switch. This is read by the screen reader when the user focuses the switch. | ||
| */ | ||
| 'aria-label'?: string; | ||
| ref?: React.RefObject<View | null>; |
There was a problem hiding this comment.
wdyt about widening ref type to support callback & forwarded refs too?
React.RefObject currently accepts only object refs so passing React.RefCallback<View> / ForwardedRef<View> results in TS error
| ref?: React.RefObject<View | null>; | |
| ref?: | |
| | React.RefObject<View | null> | |
| | ((instance: View | null) => void) | |
| | null; |
There was a problem hiding this comment.
Taken as written. Worth recording why the shorter spelling does not work here, in case it comes up again.
The repo's usual form for this prop is React.Ref<View> (Surface.tsx:56, Button.tsx:138, TouchableRipple.tsx:73), so I tried that first. It does not compile on Switch:
src/components/Switch/Switch.tsx(351,11): error TS2769: No overload matches this call.
Overload 1 of 2, '(props: ViewProps): View', gave the following error.
Type '(instance: View | null) => void | (() => VoidOrUndefinedOnly)' is not assignable to type 'Ref<View> | undefined'.
... Two different types with this name exist, but they are unrelated.
React 19's RefCallback may return a cleanup function, and @types/react declares that return type in two files (index.d.ts and ts5.0/index.d.ts, picked by typesVersions). Switch attaches the ref to a bare RN <View>, which is where the two declarations meet and fail to unify. The other components avoid it because they forward to Animated.View or to their own wrapper, not to a raw View.
Your explicit (instance: View | null) => void sidesteps that, since a void return position accepts a function returning anything. So the suggestion is used verbatim.
| await render(<Switch value ref={ref} />); | ||
|
|
||
| expect(typeof ref.current?.measure).toBe('function'); | ||
| }); |
There was a problem hiding this comment.
could we add coverage for a callback ref as well?
both current tests use React.createRef, so they only exercise object refs & wouldn’t catch this type/API regression
There was a problem hiding this comment.
Added accepts a callback ref, typed as React.RefCallback<View> so it exercises the widened type rather than only the runtime path. Switch.test.tsx goes from 12 to 13.
Verified both halves separately, since a type widening is not covered by a runtime assertion:
Type: with the new test in place but the prop still React.RefObject<View | null>, tsc -b fails with src/components/__tests__/Switch.test.tsx(104,32): error TS2322: Type '(instance: View | null) => void | (() => VoidOrUndefinedOnly)' is not assignable to type 'RefObject<View | null>'. Widening the prop clears it.
Runtime: with Switch.tsx reverted to its pre-PR state, all three ref tests fail, the new one with expect(received).not.toBeNull() / Received: null.
Widen the ref prop so callback and forwarded refs typecheck, and cover the callback ref path in tests.
Motivation
Switchaccepts noref.Propsdoes not declare one, the component signature never destructures one, and the root<View style={[styles.wrapper, style]}>is not given one — soref.currentstaysnulland there is no way tomeasure()or otherwise reach the switch's host view.This looks like a leftover from the Reanimated/
Pressablerewrite: the surrounding components carry the prop over correctly (Appbar/AppbarBackAction.tsx,FAB/Extended.tsx,Surface.tsx,List/ListItem.tsx,TouchableRipple),Switchjust did not.Scope / correction to the issue
#3729 names both
AvatarandSwitch. I checkedAvatarand it is not affected:AvatarText,AvatarIconandAvatarImageall spread...restonto their rootView, so arefpassed to them does land on the host view (I verifiedref.currentis non-null for all three). TheirPropstypes just do not advertiseref, which is a typing gap rather than a runtime bug — happy to add the declarations in a follow-up if you want.So this PR only fixes
Switch. It does not close #3729 on its own, since that issue asks for the whole surface. One thing I did notice while looking:Banner.tsxdeclaresref?: React.RefObject<View>in itsPropsbut never uses the value, soBannerdrops refs too — again, happy to do that separately.Note on the ref type
Peers use
ref?: React.Ref<View>, but those all forward to a component whose props already carryReact.RefAttributes<View>(Animated.View,Pressable,TouchableRipple).Switch's root is a plain<View>, and putting aReact.Ref<View>on it failsyarn typecheck:tsc -balso buildsdocs/, which has its own copy of@types/react(same version, separate directory), and theunique symbolbehindVoidOrUndefinedOnlyin the callback-ref signature does not unify across the two copies.React.RefObject<View | null>is structural and unaffected, matches whatReact.createRef<View>()anduseRef<View>(null)produce, and follows the existing declaration inBanner.tsx. Glad to switch it toReact.Ref<View>if you'd rather fix the duplicate-types setup instead.Test plan
Two cases added to
src/components/__tests__/Switch.test.tsx:ref.currentis notnullref.current.measureis a function (i.e. it is the host view, not some other object)Both fail before the change (
Received: null/Received: "undefined") and pass after.yarn test: 55 suites, 737 passed / 1 skipped, 169 snapshots — no snapshot churn.yarn lintandyarn typecheckclean.Refs #3729.