Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: remove NonNullable to fix recursive infer in PathConfigMap #11430

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: remove NonNullable to fix recursive infer in PathConfigMap #11430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 37 additions & 5 deletions example/__typechecks__/common.check.tsx
View file
Open in desktop
Expand Up @@ -8,11 +8,12 @@ import type {
DrawerNavigationOptions,
DrawerScreenProps,
} from '@react-navigation/drawer';
import type {
CompositeScreenProps,
NavigationAction,
NavigationHelpers,
NavigatorScreenParams,
import {
type CompositeScreenProps,
type LinkingOptions,
type NavigationAction,
type NavigationHelpers,
type NavigatorScreenParams,
} from '@react-navigation/native';
import {
createStackNavigator,
Expand Down Expand Up @@ -498,3 +499,34 @@ const FourthStack = createStackNavigator<FourthParamList, 'MyID'>();
expectTypeOf(FourthStack.Navigator).parameter(0).toMatchTypeOf<{
id: 'MyID';
}>();

/**
* Infer correct LinkingOptions
*/
export const linkingOptions: LinkingOptions<RootStackParamList> = {
prefixes: ['reactnavigation://'],
config: {
screens: {
Home: {
screens: {
Feed: {
screens: {
Popular: 'popular',
Latest: 'latest',
// @ts-expect-error "NonExistingRoute" does not exist in the FeedTabParamList
NonExistingRoute: 'non-existing',
},
},
Account: 'account',
},
},
PostDetails: 'post/:id',
Login: {
path: 'login',
// @ts-expect-error The Login route is not a nested navigator and should not have a screens object
screens: {},
},
NotFound: '*',
},
},
};
2 changes: 1 addition & 1 deletion example/src/Screens/NotFound.tsx
View file
Open in desktop
Expand Up @@ -14,7 +14,7 @@ export const NotFound = ({
<Text style={styles.title}>404 Not Found ({route.path})</Text>
<Button
variant="filled"
onPress={() => navigation.navigate('Home')}
onPress={() => navigation.navigate('Home', { screen: 'Examples' })}
style={styles.button}
>
Go to home
Expand Down
2 changes: 1 addition & 1 deletion example/src/screens.tsx
View file
Open in desktop
Expand Up @@ -76,7 +76,7 @@ export type RootDrawerParamList = {
};

export type RootStackParamList = ExampleScreensParamList & {
Home: NavigatorScreenParams<RootDrawerParamList> | undefined;
Home: NavigatorScreenParams<RootDrawerParamList>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this? Please revert to how it was

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please help me with how linking options should be defined here if the Home route is NavigationScreenParams<RootDrawerParamList> | undefined. I'm getting this typecheck error when it can be undefined:

Object literal may only specify known properties, and 'screens' does not exist in type 'Omit<PathConfig<{}>, "initialRouteName" | "screens">'

on this row

react-navigation/example/src/index.tsx

Line 69 in d0abdee

screens: {

I have no experience from using NavigatorScreenParams in a union with undefined and really no idea how it should be handled by the lib. Would really appreciate some guidance. Thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are omitting screens from the type if it's not extending NavigatorScreenParams here: https://github.com/react-navigation/react-navigation/blob/d0abdee67f5db8cf39112af535846ffededfb21d/packages/core/src/types.tsx#L1012C37-L1012C46

Since this PR removes NonNullable from that line (because NonNullable broke recursive infer) I think that makes it impossible for a route to be union of NavigatorScreenParams and undefined?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it makes it impossible to have undefined then it'd be a breaking change. The change needs to support | undefined to be merged

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but if a route is defined as NavigatorScreenParams<{}> | undefined should you be able to config linking options for the nested navigator as in the example?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't make sense to support | undefined if it only works with some of the things and not others.

NotFound: undefined;
};

Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/types.tsx
View file
Open in desktop
Expand Up @@ -1005,9 +1005,9 @@ export type PathConfig<ParamList extends {}> = {
};

export type PathConfigMap<ParamList extends {}> = {
[RouteName in keyof ParamList]?: NonNullable<
ParamList[RouteName]
> extends NavigatorScreenParams<infer T extends {}>
[RouteName in keyof ParamList]?: ParamList[RouteName] extends NavigatorScreenParams<
infer T
>
? string | PathConfig<T>
: string | Omit<PathConfig<{}>, 'screens' | 'initialRouteName'>;
};