Create getParam helper

[peterpme]

Long time listener, first time caller.

I find myself stumbling over this.props.navigation.state.params far too often. After searching the issues, I found https://github.com/react-navigation/react-navigation/pull/3510 which led me to DM Brent which led me here.

I apologize if version 1 does not include Detailed design. I have a lot to learn about react-navigation. I just wanted to put this up for now.

If that doesn't work, you are welcome to close it and I'll re-open when I'm more familiar with the codebase.

Thanks!

[ericvicenti]

Maybe we could make it so that each screen gets props.params, in addition to props.navigation. This would really just be a shortcut for this.props.navigation.state.params || {}

[peterpme]

That's a great idea! I would prefer that instead of deprecating the current implementation.

[dantman]

What about navigationOptions functions and all the code that already gets passed a copy of props.navigation and now instead of being refactored to use navigation.getParam instead of navigation.state.params || {} needs major refactoring to also get passed props.params no mater how many components in between the Route and the component using it need to be updated to do so.

Rather than a new prop or a helper I wish we'd just go with the original fix everyone in the bug was discussing before someone popped in ages later and suggested a getParam helper instead. Just fix React Navigation so that params is always an object, never defaulting to undefined when params aren't set during the navigation.

[ericvicenti]

Yeah, @dantman, thats a great point. Ok ok, I hope this makes everybody happy:

On the screen navigation prop, provide navigation.params which is equivalent to navigation.state.params || {} . This allows for the following safe usage:

const {foo, bar} = this.props.navigation.params;

cc @brentvatne, I think this would be a substantial usability improvement for people. It would be a one line change after we land the new createNavigator API.

BTW, I think this is a small enough change that we can do this without a whole RFC. Next time you have a simple idea like this @peterpme, it might be easier to create an issue in this repo and we can move fast on it 😁

[dantman]

Ok. Just keep in mind that || {} should not actually be used, as the {} might break PureComponent optimizations if someone passes params on to a component and they are undefined.

To prevent that from being an issue the file that defines the navigation.params helper should have a const emptyParams = Object.freeze({}); somewhere at the root to create a common emptyParams empty object that can't be modified. And then the params getter would use navigation.state.params || emptyParams.

[peterpme]

Sounds great to me! Thanks for the discussion, I don't mind implementing this!

Do ya'll still want to merge in https://github.com/react-navigation/react-navigation/pull/3510 since its complete and I can get started on making params default to an empty object as well?

[ericvicenti]

Wait, do we really need getParams, once we have navigation.params?

Also, can you try implementing it on this branch? It should be way way easier

https://github.com/react-navigation/react-navigation/tree/%40ericvicenti/navigator-awesome

[peterpme]

We don't, but Brent suggested it 😝

I'd be happy to just implement navigation.params on navigator-awesome instead 😄

Thanks for the opportunity!

[peterpme]

I’ve gone over the new branch and I think I figured it out but it seems too easy.

My only hesitation comes from this new branch and despite tests passing I think it’s still WIP?

Two places where I think I can set the default params value to:

addNavigationHelpers createNavigator

My tests all pass besides the snapshots (obv) but I feel like there’s more I’m missing!

I’ve branched off awesome-navigator and will push up in the am

Thanks!

[satya164]

I would really like params as a direct prop (this.props.params) instead of this.props.navigation.params. To me, it's not that different from this.props.navigation.state.params.

Regarding navigationOptions, I talked to @brentvatne earlier about making it possible to configure it inside the component instance (.setOptions) which will make it easier to use params when configuring options.

[dantman]

I've commented in https://github.com/react-navigation/rfcs/issues/24#issuecomment-365992743 about why we can't have setOptions, so my problem with a props.params that is not available to the navigationOptions function still stands.

[brentvatne]

@dantman - does @ericvicenti's comment about still passing it in via navigation.params in addition to props.params handle that for you?

Things we know for sure

• params should be an empty object when there are no params, not undefined
• navigation.state.params is unnecessarily verbose, we should move it to navigation.params

Options

Each option takes for granted that we will be doing the "things we know for sure"

1. Pass params in as prop. this.props.params.name seems pleasant, but it increases API surface area (perhaps confusing to see params in two places without having any background on why).
2. Add navigation.getParam helper. this.props.navigation.getParam('name', fallbackValue). The main benefit of this is it is a clean way to provide a default value (if null is passed in, null is used, if nothing is passed in, fallback is used. these sorts of checks are annoying to do everywhere).
3. getParam handles default values well, but another more declarative way to handle this would be to add a defaultParams static.
4. Do nothing beyond the "things we know for sure".

[ericvicenti]

1. I think the disadvantage here is not the increased surface area, but that people pass around their navigation prop, and they would wonder, should I also pass around the params?
2. Fallback value seems reasonable, but personally I'd be fine with just doing || fallback in my code
3. I'd like to push back on this until we see a use case that demands it
4. I'm advocating for this option, but it still leaves ambiguity: (I could go either way on this) 4a. Navigation state.params may still be missing, but navigation.params is always an object 4b. Navigation state.params AND navigation.params are always objects

[brentvatne]

@ericvicenti - based on your concerns re: 4 -- perhaps we don't actually know for sure that we want to move navigation.state.params up to navigation.params then. duplicating a key is always a bit confusing. this leads me to think that getParam isn't a bad option after all -- we can get the benefits of having the params more easily accessible without pointing to params from multiple places and with slightly better ergonomics for no params / default values.

[ericvicenti]

Good point, I agree that props.navigation.getParam() is likely to be the least confusing API we could provide at this time, plus it enables that default behavior. If others agree then lets go for it!

[satya164]

But is this.props.navigation.getParam('foo', 'bar') really that shorter than this.props.navigation.state.params.foo || 'bar'? It literally saves me 3 characters! In case of this.props.navigation.getParam('foo') and this.props.navigation.state.params.foo, it saves me 1 character.

I agree that we should initialize params to an empty object by default. IMO the addition of a new getParam API doesn't provide any advantages if we do that.

I like this.props.params is much shorter and convenient to use. I think it's reasonable even if it increases the API surface area, but I totally understand if you don't want that.

[brentvatne]

[dantman]

What I was writing before brent posted.

• I agree that this.props.navigation.state.params.foo would be nice. But the reason that's a problem is because state.params is not guaranteed to be an object. I do agree that the nicest solution (the original one) would be to initialize params to an empty object. However no one has come up with a PR or suggested how to do that. To my understanding we're talking about alternative APIs because that solution isn't easy to implement. I believe it requires a change to every NavigationAction or a change to every Router's getStateForAction.
• this.props.navigation.state.params.foo || 'bar' and this.props.navigation.getParam('foo', 'bar') are not the same thing. If foo is 0, false, or null and that is a valid value you want, then || doesn't work the same way as a defaultParam arg to getParam.
• this.props.params is not any way convenient to use if you already have code that passes around navigation, which is a completely normal pattern to have.

[peterpme]

What do people use params most frequently for? Personally, these 2 scenarios:

1: pass id to next screen 2: pass id and title to next screen

props.params would be most convenient when I have more than 1:

const { id, title } = this.props.params

I don't mind this with more than 1:

const id = this.props.navigation.getParam('id')
const title = this.props.navigation.getParam('title')

but prefer option 1

I do agree with @dantman about refactoring your code to support this.props.params instead though. Not really sure how people use params outside of my bubble.

[satya164]

@peterpme also

const { id, title = 'Some title' } = this.props.navigation.state.params;

[dantman]

Devil's advocate, technically speaking the destructuring syntax doesn't have much of a problem, it's already trivial to bypass the undefined params issue.

const { id, title } = this.props.navigation.state.params || {};
// or
const { navigation } = this.props;
const { id, title } = navigation.state.params || {};

The biggest issue we have is use of a single param in methods, i.e. short one-off methods like event handlers or callbacks that need access to a single param but the undefined params issue forces them to be much longer than they need to.

onClick = () => doSomething({force: this.props.navigation.state.params && this.props.navigation.state.params.isAdminPage});

This is why I didn't object to getParam. It would have been nice to guarantee that this.props.navigation.state.params is an object, however I understood that getParam was much easier to implement, didn't have the problem of having two different things called params, and it solved the biggest problem.

onClick = () => doSomething({force: this.props.navigation.getParam('isAdminPage')});

[peterpme]

Now for the fun part! I will make this getter happen.

Just one question. Where?

• @ericvicenti's awesome-navigator branch
• master branch

[brentvatne]

@dantman - that's a really great point. @satya164 - what do you think?

[dantman]

;) keep in mind that's a devils advocate comment because I don't like having to do || {}; all the time.

I do still wish it would be possible to guarantee navigation.state.params is an object. Even though it's probably the most work to solve. (Although I don't think it's as impossible due to user-routers as I used to think it was)

[satya164]

regarding navigation.state.params || {};, while it's possible to do this for undefined params, I think it's still nice that we return an empty object. because it addresses a common use case without increasing the API surface. so while we don't lose anything, we make it a tiny bit nicer to use, especially with that one-off method example.

[brentvatne]

To unblock this discussion, I propose that we proceed with this:

• Add getParam as per @peterpme's existing PR
• Investigate a good approach for making navigation.state.params default to object (as @dantman pointed out this is complicated a bit by custom routers). @peterpme perhaps you could look into it?
• In the future, we may hoist more things up to top level props (eg: this.props.params). At the moment we have higher impact issues to address.

If that sounds reasonable, @peterpme can you make this RFC reflect the getParam approach and we'll move forward with that?

[peterpme]

Works for me!

• RFC will now reflect getParam approach
• I'll investigate navigation.state.params defaulting to object

Thanks everybody ❤️

[dantman]

@brentvatne My original thought was that to guarantee params are {} by default the only way to do this would be to make the getStateForAction of every router and custom router defaults params to {}.

However I did think of an alternative recently. The NavigationActions functions already have code like action.params = payload.params it wouldn't be hard to make sure params is an object there. Then instead of ensuring all routers set params to an object we'd just have to ensure all NavigationActions that add a route set params to an object. Though perhaps that's not much easier and isn't as guaranteed as making the router do it.

[peterpme]

Before I get too far with trying to get tests to pass, I wanted to see if I'm going down the right path for setting params default to {}

https://github.com/react-navigation/react-navigation/pull/3562