react-navigation / rfcs

RFCs for changes to React Navigation
Other
88 stars 16 forks source link

Add getParams as Navigation prop #63

Closed cbrevik closed 4 years ago

brentvatne commented 5 years ago

This looks nice to me, I would use this API. @ericvicenti - what do you think?

LinusU commented 5 years ago

Another approach could be to default navigation.state.params to {} if no params were provided. Actually, regardless of whether we add this function or not, I still think that would be a good idea.

I personally prefer consice, predicatable and simple data structures in favour of helper functions for accessing different parts.

An example that gets unnessecarily complicated with todays design is:

type AccountPageNavigationProps = (
  { mode: 'add' } |
  { mode: 'edit', id: string } |
  { mode: 'view', id: string } |
  { mode: 'import', inboxEntryId: string }
)

interface AccountPageProps {
  navigation: NavigationScreenProp<{}, AccountPageNavigationProps>
  screenProps: { vault: Vault }
}

export default class AccountPage extends Component<AccountPageProps> {
  // ...
}

Currently, I have to dance around the fact that navigation.state.params isn't typed as AccountPageNavigationProps, and also that it can potentially be undefined.

cbrevik commented 5 years ago

@LinusU Maybe I'm misunderstanding your example here. But if I copy-paste that code into VS Code, then navigation.state.params seems to be typed for me?:

image

Anyway, to your point of defaulting navigation.state.params to {}, might that constitute a breaking change for cases where people expect undefined?

I don't know the react-navigation internals well enough to comment on if that would be easy or difficult to implement though.

LinusU commented 5 years ago

@cbrevik you have to use "strict": true in your tsconfig.json in order to notice the problem. Then it will be typed as AccountPageNavigationProps | undefined ☺️

cbrevik commented 5 years ago

Alright, that would be because of strictNullChecks and the fact that params is optional. Was confused by the wording of the last sentence in your previous comment. 👍

satya164 commented 4 years ago

Closing since getParam was implemented and available in v3 and v4.

For v5, I believe nullish coalescing operator and optional chaining operator can provide the same benefits. It's shorter to type too since screens receive a route prop (so route.params?.title ?? 'Some title')