refactor(Tabs): Split JS components and specs internally#3722
refactor(Tabs): Split JS components and specs internally#3722
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the TabsHost/TabsScreen implementation to enforce platform separation (iOS vs Android) across JS components, TS types, and native/Fabric bindings, aligning with the RFC’s direction and making platform-specific props explicit.
Changes:
- Split
TabsHost/TabsScreeninto platform-specific JS entrypoints (.ios.tsx/.android.tsx) and platform-specific Fabric codegen specs. - Reorganized the public TS API by moving platform-only props into
ios/androidnested objects and namespacing platform type exports. - Updated native iOS/Android registration and conversions to use platform-specific component names (e.g.
RNSTabsHostIOS/RNSTabsHostAndroid) and adjusted example/test apps accordingly.
Reviewed changes
Copilot reviewed 47 out of 49 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/fabric/tabs/TabsScreenIOSNativeComponent.ts | iOS-only Fabric spec cleanup + renamed component to RNSTabsScreenIOS. |
| src/fabric/tabs/TabsScreenAndroidNativeComponent.ts | New Android-only Fabric spec (RNSTabsScreenAndroid). |
| src/fabric/tabs/TabsHostIOSNativeComponent.ts | iOS-only Fabric spec + renamed component to RNSTabsHostIOS. |
| src/fabric/tabs/TabsHostAndroidNativeComponent.ts | New Android-only Fabric spec (RNSTabsHostAndroid). |
| src/components/tabs/useTabsScreen.ts | Extracted shared TabsScreen lifecycle/debug logic into a hook. |
| src/components/tabs/useTabsHost.ts | Extracted shared TabsHost focus/debug logic into a hook. |
| src/components/tabs/index.ts | Exposes new namespaced platform type exports. |
| src/components/tabs/TabsScreen.types.ts | Moves platform props into ios / android sub-objects. |
| src/components/tabs/TabsScreen.tsx | Removed monolithic cross-platform implementation. |
| src/components/tabs/TabsScreen.ios.types.ts | New iOS-only TabsScreen types. |
| src/components/tabs/TabsScreen.android.types.ts | New Android-only TabsScreen types. |
| src/components/tabs/TabsScreen.ios.tsx | New iOS TabsScreen implementation wiring iOS-only props. |
| src/components/tabs/TabsScreen.android.tsx | New Android TabsScreen implementation wiring Android-only props. |
| src/components/tabs/TabsScreen.d.ts | TS shim for extensionless imports (./TabsScreen). |
| src/components/tabs/TabsHost.types.ts | Moves platform props into ios / android sub-objects. |
| src/components/tabs/TabsHost.tsx | Removed monolithic cross-platform implementation. |
| src/components/tabs/TabsHost.ios.types.ts | New iOS-only TabsHost types (incl. bottom accessory types). |
| src/components/tabs/TabsHost.android.types.ts | Placeholder Android-only TabsHost types. |
| src/components/tabs/TabsHost.ios.tsx | New iOS TabsHost implementation wiring iOS-only props + bottom accessory. |
| src/components/tabs/TabsHost.android.tsx | New Android TabsHost implementation. |
| src/components/tabs/TabsHost.d.ts | TS shim for extensionless imports (./TabsHost). |
| package.json | Updates iOS codegen component map to RNSTabsHostIOS / RNSTabsScreenIOS. |
| ios/tabs/screen/RNSTabsScreenEventEmitter.mm | Updates event emitter types for iOS-specific codegen symbols. |
| ios/tabs/screen/RNSTabsScreenEventEmitter.h | Header updates for iOS-specific event emitter types. |
| ios/tabs/screen/RNSTabsScreenComponentView.mm | Uses iOS-specific Props/EventEmitter/Descriptor types. |
| ios/tabs/host/RNSTabsHostEventEmitter.mm | Updates host event emitter type to iOS-specific codegen symbol. |
| ios/tabs/host/RNSTabsHostEventEmitter.h | Header updates for iOS-specific host event emitter type. |
| ios/tabs/host/RNSTabsHostComponentView.mm | Uses iOS-specific Props/EventEmitter types and enum names. |
| ios/conversion/RNSConversions.h | Updates conversion function signatures to iOS-specific codegen enums. |
| ios/conversion/RNSConversions-Tabs.mm | Updates conversion implementations to iOS-specific codegen enums. |
| common/cpp/react/renderer/components/rnscreens/RNSTabsHostShadowNode.h | Selects platform-specific Props/EventEmitter types via #if !defined(ANDROID). |
| common/cpp/react/renderer/components/rnscreens/RNSTabsHostShadowNode.cpp | Makes host component name platform-specific (RNSTabsHostIOS/Android). |
| android/.../tabs/screen/TabsScreenViewManager.kt | Renames manager interface/delegate + module name to Android-specific. |
| android/.../tabs/host/TabsHostViewManager.kt | Renames manager interface/delegate + module name to Android-specific. |
| apps/src/tests/single-feature-tests/tabs/test-tabs-appearance-defined-by-selected-tab.tsx | Updates examples to use new ios/android prop nesting + namespaced types. |
| apps/src/tests/single-feature-tests/tabs/override-scroll-view-content-inset.tsx | Moves iOS-only prop under tabScreenProps.ios. |
| apps/src/tests/issue-tests/TestSafeAreaViewIOS/split-view/ConfigColumn.tsx | Updates icon props nesting for iOS. |
| apps/src/tests/issue-tests/TestSafeAreaViewIOS/bottom-tabs/BottomTabsComponent.tsx | Moves iOS-only props under ios and host props under BottomTabsContainer.ios. |
| apps/src/tests/issue-tests/TestHeaderHeight.tsx | Updates per-platform icon props nesting. |
| apps/src/tests/issue-tests/TestBottomTabsOrientation.tsx | Updates per-platform icon + orientation nesting. |
| apps/src/tests/issue-tests/TestBottomTabs/index.tsx | Updates appearance/icon props to new per-platform nesting. |
| apps/src/tests/issue-tests/Test3596.tsx | Updates icon nesting + formatting. |
| apps/src/tests/issue-tests/Test3443.tsx | Updates iOS icon nesting. |
| apps/src/tests/issue-tests/Test3342.tsx | Moves iOS-only props under ios. |
| apps/src/tests/issue-tests/Test3288.tsx | Moves iOS-only props (incl. bottom accessory) under BottomTabsContainer.ios. |
| apps/src/tests/issue-tests/Test3236.tsx | Moves controller mode under BottomTabsContainer.ios. |
| apps/src/tests/issue-tests/Test3212/StackInBottomTabsScenario.tsx | Moves scroll edge effects under tabScreenProps.ios. |
| apps/src/tests/issue-tests/Test3212/BottomTabsScenario.tsx | Moves scroll edge effects under tabScreenProps.ios. |
| apps/src/tests/issue-tests/Test3168.tsx | Moves iOS-only systemItem under tabScreenProps.ios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 49 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kligarski
left a comment
There was a problem hiding this comment.
Looks great, I left only some minor nitpicks and suggestions.
One more question: what about bottom accessory: should it also have excludedPlatforms in spec file? Should the name be changed to TabsBottomAccessoryIOSNativeComponent or if this is already iOS-only component we don't need IOS part?
| /** | ||
| * @summary Specifies the color scheme used by the container and any child containers. | ||
| * | ||
| * The following values are currently supported: | ||
| * - `inherit` - the interface style from parent, | ||
| * - `light` - the light interface style, | ||
| * - `dark` - the dark interface style. | ||
| * | ||
| * @default inherit | ||
| * | ||
| * @platform ios | ||
| */ | ||
| colorScheme?: TabsHostColorScheme; |
There was a problem hiding this comment.
This will be also available on Android soon but I'll move it in #3723 after we merge this PR.
There was a problem hiding this comment.
Hmm, haven't we agree to lift all props planned for both platforms immediately up? https://github.com/software-mansion/react-native-screens-labs/discussions/1021#discussioncomment-15982501
If you put them here, then the PR with colorScheme support will block the stabilization. If you hoist it, then the colorScheme PR won't be blocking. I know that it's basically ready, but just from the organisational PoV.
There was a problem hiding this comment.
Because we should be consistent for all such props
There was a problem hiding this comment.
We just wanted this PR to be merged as quickly as possible and I didn't want to make @t0maboro rebase for the 100th time. For me it doesn't make any difference - it can be moved to shared now or later.
There was a problem hiding this comment.
+1 ^
We may consider merging #3723 first if we want to have a final shape here, but with a single prop, it would be less probable to make any error during rebase inside that PR.
There was a problem hiding this comment.
okay, let's proceed with this one and orientation then
common/cpp/react/renderer/components/rnscreens/RNSTabsHostShadowNode.h
Outdated
Show resolved
Hide resolved
common/cpp/react/renderer/components/rnscreens/RNSTabsHostShadowNode.cpp
Outdated
Show resolved
Hide resolved
sure, we may consider it
the only reason behind adding these suffixes was that codegen was iterating over files and generating duplicated symbols, when the name was |
I understand but I wonder if maybe doing so even if it's not necessary by codegen would be better or not - advantage is that it would all be consistent across all files (e.g. in the future, if we have some files that have shared codegen spec, they wouldn't have any suffix but iOS-specific accessory would) but I guess that it is a bit redundant so I'm okay with both solutions. |
not a strong opinion, but I really don't like the fact that right now we're forced to have iOS in the name, e.g. |
|
kkafar
left a comment
There was a problem hiding this comment.
Great job.
Left few remarks & questions. Please answer them.
| findTabScreenOptions(tabsConfig, 'Tab1')?.tabScreenProps.ios | ||
| ?.orientation ?? 'undefined' |
There was a problem hiding this comment.
This won't scale well. We need to refactor the BottomTabsContainer implementation.
I think this test won't be iOS only for a long time.
Not in scope of this PR obviously.
There was a problem hiding this comment.
Could you provide some more details about that refactor?
https://github.com/software-mansion/react-native-screens-labs/issues/1040
| ios: { | ||
| orientation: value === 'undefined' ? undefined : value, | ||
| }, |
There was a problem hiding this comment.
I have an immediate question remark here though:
IIRC the version we settled on has been this one - orientation should be in general section.
| export default SCENARIO; | ||
|
|
||
| const DEFAULT_APPEARANCE_ANDROID: TabsScreenAppearanceAndroid = { | ||
| const DEFAULT_APPEARANCE_ANDROID: TabsScreenPropsAndroid.TabsScreenAppearance = { |
| <View style={styles.section}> | ||
| <Text style={styles.heading}>TabsHost color scheme</Text> | ||
| <SettingsPicker<NonNullable<TabsHostProps['colorScheme']>> | ||
| <SettingsPicker<NonNullable<TabsHostPropsIOS.TabsHostColorScheme>> |
There was a problem hiding this comment.
ColorScheme also should not be iOS only, right @kligarski?
| /** | ||
| * TS module resolution does not support this RN platform extension pattern out of the box. | ||
| * Without a base .tsx file, TS will throw a "Cannot find module" error. | ||
| * | ||
| * This file satisfies the TS compiler by providing the correct type signatures, | ||
| * whereas Metro will handle the proper runtime file resolution. | ||
| */ |
| /** | ||
| * @summary Specifies the color scheme used by the container and any child containers. | ||
| * | ||
| * The following values are currently supported: | ||
| * - `inherit` - the interface style from parent, | ||
| * - `light` - the light interface style, | ||
| * - `dark` - the dark interface style. | ||
| * | ||
| * @default inherit | ||
| * | ||
| * @platform ios | ||
| */ | ||
| colorScheme?: TabsHostColorScheme; |
| /** | ||
| * @summary Specifies the color scheme used by the container and any child containers. | ||
| * | ||
| * The following values are currently supported: | ||
| * - `inherit` - the interface style from parent, | ||
| * - `light` - the light interface style, | ||
| * - `dark` - the dark interface style. | ||
| * | ||
| * @default inherit | ||
| * | ||
| * @platform ios | ||
| */ | ||
| colorScheme?: TabsHostColorScheme; |
There was a problem hiding this comment.
Because we should be consistent for all such props
| export type TabsScreenBlurEffect = | ||
| | 'none' | ||
| | 'systemDefault' | ||
| | 'extraLight' | ||
| | 'light' | ||
| | 'dark' | ||
| | 'regular' | ||
| | 'prominent' | ||
| | 'systemUltraThinMaterial' | ||
| | 'systemThinMaterial' | ||
| | 'systemMaterial' | ||
| | 'systemThickMaterial' | ||
| | 'systemChromeMaterial' | ||
| | 'systemUltraThinMaterialLight' | ||
| | 'systemThinMaterialLight' | ||
| | 'systemMaterialLight' | ||
| | 'systemThickMaterialLight' | ||
| | 'systemChromeMaterialLight' | ||
| | 'systemUltraThinMaterialDark' | ||
| | 'systemThinMaterialDark' | ||
| | 'systemMaterialDark' | ||
| | 'systemThickMaterialDark' | ||
| | 'systemChromeMaterialDark'; |
There was a problem hiding this comment.
Leaving a note here to remember to refactor this according to the discussion.
There was a problem hiding this comment.
| } else { | ||
| componentNodeHandle.current = -1; | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
This can be replaced with useRenderDebugInfo in general I think (another PR of course).
| // Namespace the Host platform-specific types | ||
| export type * as TabsHostPropsAndroid from './TabsHost.android.types'; | ||
| export type * as TabsHostPropsIOS from './TabsHost.ios.types'; | ||
|
|
||
| // Namespace the Screen platform-types | ||
| export type * as TabsScreenPropsAndroid from './TabsScreen.android.types'; | ||
| export type * as TabsScreenPropsIOS from './TabsScreen.ios.types'; |
There was a problem hiding this comment.
Leaving a note here for future PRs - this wasn't settled. I don't like namespaceing here. I don't know any other big lib that does this. Let's see how this will look in the final PR.
kkafar
left a comment
There was a problem hiding this comment.
If we decide to leave orientation & colorScheme as platform specific right now, then let's proceed.
|
Please update the PR description before merging. Right now you'll close only one of the issues. To close both, you need to repeat |
@kkafar orientation moved up, because there's no PR related to it atm, |
updated |
## Description Omission from #3722 (comment) ## Changes - fixed icon - removed badge, as no badges are displayed ## Checklist - [ ] Included code example that can be used to test this change. - [ ] Updated / created local changelog entries in relevant test files. - [ ] For visual changes, included screenshots / GIFs / recordings documenting the change. - [ ] For API changes, updated relevant public types. - [ ] Ensured that CI passes
Important notes before reviewing this PR:
standardAppearanceAndroid->standardAppearanceDescription
This PR introduces a platform separation for the
TabsHostandTabsScreencomponents. The platform-specific code is now placed in a separate file for each platform.From now on, platform-specific props should not be passed to the native side of components on other platforms. If a prop will have an empty implementation on the native side, it means that it was defined in the wrong location.
Caution
This is a breaking change to the public API.
Caution
Since we're no longer supporting old architecture, I didn't take any steps to synchronize these changes in it. In fact, the app most likely won't even build on Paper at this point.
Closes: https://github.com/software-mansion/react-native-screens-labs/issues/997
Closes: https://github.com/software-mansion/react-native-screens-labs/issues/998
Changes
From this PR:
TabsX.tsxwithTabsX.ios.tsxandTabsX.android.tsxfiles.iosandandroidobjects and created dedicated files for platform-specific props.useTabsXhooks.RNSTabsHostShadowNoderequires a platform-specific name - without that, app is crashing because the Host component hasn't been found in the registry.react::RNSTabsScreenX->react::RNSTabsScreenIOSXcomponentNodeRef, which before this PR was present in shared code forTabsXcomponent (viauseTabsXhook) was moved to the component's implementation, because types have diverged between platforms after splitting native componentsBefore & after - visual documentation
N/A - refactor, all behaviors should remain unchanged
Test plan
Go through all updated examples included in this PR
Checklist