router.getScreenConfig is broken

[satya164]

Currently the second argument received by a navigation options item is the previous config returned from a parent navigation options, and the options passed to getScreenConfig.

IMO it's a confusing behaviour, because fundamentally they are different types of objects, one is the configuration for navigator, other is arbitrary options the navigator can pass to the configuration function. For example, the tab navigator can provide a focused argument for the icon, which doesn't make sense in a config.

The way this works is, only the global navigator config receives the options passed to the getScreenConfig, and it has to explicitly merge it with, say, header config and return it. Which means if you have a static header configuration for screens, and then add a header configuration at the global level to be used as default, it breaks the screen configuration because it doesn't receive the options anymore and you've to explicitly merge the options in the global navigator config. Same for the route navigation config, which will break if you add a screen config.

Personally I think the confusing behaviour and the learning curve associated with it is not worth getting rid of one extra function.

global config - the navigationOptions passed in the second argument to the navigator
screen config - the static navigationOptions in the screens
route config - the navigationOptions specified for individual routes in the first argument to the navigator

Flow types for both,

type NavigationOptionItem<T> =
  | T
  | (navigation: NavigationScreenProp<*,*>, T) => T

type IconOptions = {
  focused: boolean;
  tintColor: boolean;
}

These are the current api and the proposals we discussed:

Current API

Options is the result of previous header function/previous header object

header: (navigation, options) => ({
  ...options,
  icon: ({ focused, tintColor }) => <Icon name={'settings' + focused ? '' : '-outline'} color={tintColor} />
});

Flow type,

type HeaderOptionItem = NavigationOptionItem<{
  title: string;
  icon: IconOptions => React.Element<*>;
}>

How it is used,

const { icon } = router.getScreenConfig(...);
return icon({ focused, tintColor });

Pros

• router.getScreenConfig doesn't deal with options passed to icon, so it's pretty generic, navigators can do whatever they want
• the icon function can be declared inline, so much simpler to use
• we can support both function and plain element, so no one needs to use a function if not needed

Cons

• Function inside object inside function, though it's function inside object if you don't need the outer function

Proposed API #1

Options is the result of previous header function/previous header object merged with params passed to getScreenConfig

header: (navigation, options) => ({
  ...options,
  icon: <Icon name={'settings' + options.focused ? '' : '-outline'} color={options.tintColor} />
});

Flow type,

type HeaderOptionItemConfig = {
  title: string;
  icon: React.Element<*>;
}

// not sure if `HeaderOptionItemConfig & ?IconOptions` is valid, but wanted to denote that the merged object may or may not contain icon options even if the navigator always passes it
type HeaderOptionItem = NavigationOptionItem<HeaderOptionItemConfig & ?IconOptions>

How it is used,

return router.getScreenConfig(.., { focused, tintColor }).icon;

Pros

• Only two arguments to learn

Cons

• Automatically merging 2 different kinds of objects can be confusing, for example focused doesn't make sense inside navigationOptions.header. For some things like tintColor, it could be irrelevant if tintColor might make sense in the config, but see next one for that
• getScreenConfig has to handle the options, limited to objects only
• What takes precedence? navigationOptions or the params passed to getScreenConfig? Can I override the other?

Proposed API #2

Options is the result of previous header function/previous header object

header: (navigation, options) => ({
  ...options,
  activeTintColor: 'orange',
  inactiveTintColor: 'blue',
  activeIcon: <Icon name='settings' color='blue' />,
  inactiveIcon: <Icon name='settings-outline' color='orange' />
});

Flow type,

type HeaderOptionItem = NavigationOptionItem<{
  title: string;
  activeTintColor: string;
  inactiveTintColor: string;
  activeIcon: React.Element<*>;
  inactiveIcon: React.Element<*>;
}>

How it is used,

return router.getScreenConfig(..).activeIcon;
return router.getScreenConfig(..).inactiveIcon;

Pros

• router.getScreenConfig doesn't deal with options passed to icon

Cons

• I can configure the colors of text with activeTintColor, but for the icon, I have to type it again, and I made a typo
• I have to write almost similar component twice, can make it a function to simpify, but then it's no better than current API where I can use a function, and even write it inline
• This assumes that the navigator doesn't need to pass options, and this assumption will likely break in future

Proposed API #3

Previous result is the result of previous header function/previous header object

header: (navigation, params, previousResult) => ({
  ...previousResult,
  icon: <Icon name={'settings' + params.focused ? '' : '-outline'} color={params.tintColor} />
});

Flow type,

type NavigationOptionItem<T, U> =
  | T
  | (navigation: NavigationScreenProp<*,*>, T, U) => T

type HeaderOptionItem = NavigationOptionItem<{
  title: string;
  icon: React.Element<*>;
}, IconOptions>

How it is used,

return router.getScreenConfig(.., { focused, tintColor }).icon;

Pros

• No automatic merging between incompatible types

Cons

• 3 arguments to learn (should be an object most prolly)
• getScreenConfig has to handle the options, limited to objects only, e.g. - getScreenConfig(..., () => {}) is not possible (likely not a significant issue)

cc @skevy @mkonicek

[grabbou]

Have we end up discussing something yesterday? From Slack discussion it's unclear to me whether this is still a bug and will get re-implemented.

[satya164]

@grabbou we didn't reach any conclusion. there's a small document which you should read and comment your opinion.

[ericvicenti]

I'm a fan of #1 but happy to discuss. I like that option because there are no options to the screen config, just defaults. This is a simpler approach IMO

[grabbou]

I am leaning towards the last approach which seems to be a natural progression of approach 2. Wiith a signature like this:

header: (navigation, header, params)

Personally I think it might be easier for user to have an explicit access to defaults that are loaded from parent source and params (options) that were explicitly passed to the function, when e.g. accessing an icon for a focused state.

Not really sure if merging these two kinds of objects is a good idea since at some point, e.g. one might want to pass an option that is not part of navigationOptions.tabBar and this will make people confused.

The biggest problem that I see with merging these two objects is that there's that assumption (at least I have it, I might be wrong) that options (second parameter) are options that I specify, either on component inside static navigationOptions or as defaults per navigator. Merging them with arbitrary params that library passes breaks that assumptions, since it's not me who is responsible for specifying these values.

[mkonicek]

In your example 2 the user would expect the options to be:

{
  foo: 'bar',
  tintColor: 'blue',
}

Correct?

[satya164]

In your example 2 the user would expect the options to be:

Only if the user returns a merged object,

[mkonicek]

I think I don't understand very well the problem described in this issue. I think the code example 2 at the beginning illustrates what's wrong currently but I don't understand it very well.

From the description:

Currently the second argument received by a navigation options item is the previous config returned from a parent navigation options, and the options passed to getScreenConfig.

How can the second argument be two things at once: (the previous config returned from a parent navigation options) and (the options passed to getScreenConfig)?

For example, the tab navigator can provide a focused argument for the icon, which doesn't make sense in a config.

OK this I understand. Yes that's true. But maybe it doesn't matter we merge those two things into a single options object? Is this an issue just because it's difficult to declare good Flow types for the options?

Personally I think the confusing behaviour and the learning curve associated with it is not worth getting rid of one extra function.

Do you mean learning curve for contributors or for users of the library? There are many more users of the library so if the high-level API is simpler but as a tradeoff the implementation / Flow types are more complex that's an OK tradeoff. Ideally both API and implementation should be simple of course when possible.

The issue title says it's broken. What's the bug? Is there a bug in the Example 2 that the tintColor set on the navigator gets lost?

---

In Proposal 1:

header: (navigation, options) => ({
  ...options,
  icon: <Icon name={'settings' + options.focused ? '' : '-outline'} color={options.tintColor} />
});

• Why pass navigation? It's not used. When would it be used? I guess for rendering a right button that can navigate for example, right?
• It's not ideal the user has to repeat ...options - this is just some boilerplate to write. Can the icon option be merged automatically with the navigator's options?

---

Proposal 2: I don't understand from the code examples how it's different from 1?

---

Proposal 3:

header: (navigation, params, previousResult) => ({
  ...previousResult,
  icon: <Icon name={'settings' + params.focused ? '' : '-outline'} color={params.tintColor} />
});

What is previousResult and how is it different from the option in Proposal 1? As a user I just have to copy it as ...previousResult but other than that it doesn't seem to have a purpose. Can the library do this boilerplate work so I can write:

header: (navigation, options) => ({
  icon: <Icon name={'settings' + options.focused ? '' : '-outline'} color={options.tintColor} />
});

And the React Navigation library would do the merging of screen-navigationOptions and navigator-navigatorOptions for me. Basically it would work correctly without any API change.

[satya164]

Why pass navigation? It's not used. When would it be used?

It can be used for back button, or any button that changes navigation state such as updating params etc.

It's not ideal the user has to repeat ...options - this is just some boilerplate to write. Can the icon option be merged automatically with the navigator's options?

We had a discussion on slack regarding this https://reactnativecore.slack.com/archives/navigation/p1485457433002182

Proposal 2: I don't understand from the code examples how it's different from 1.

It has different config options, for example instead of a tintColor and focused argument, it has activeIcon and inactiveIcon, and getScreenConfig doesn't pass any options.

What is previousResult and how is it different from the option in Proposal 1

previous result is

type HeaderOptionItemConfig = {
  title: string;
  icon: IconOptions => React.Element<*>;

options is

type IconOptions = {
  focused: boolean;
  tintColor: boolean;
}

options in proposal 1 is

HeaderOptionItemConfig & ?IconOptions

How can the second argument be two things at once: (the previous config returned from a parent navigation options) and (the options passed to getScreenConfig)?

Because they are merged together

HeaderOptionItemConfig & ?IconOptions

Just from reading this issue to me it seems like an API like this would work too?

This will work as long as the user returns a merged object. Also if the library merges them automatically, it doesn't need to be passed in the arguments. And merging these 2 and passing in argument means what I wrote in #1 which is confusing behaviour, merging options passed from the library with the header config defined in navigationOptions.

[satya164]

Do you mean learning curve for contributors or for users of the library? There are many more users of the library so if the high-level API is simpler but as a tradeoff the implementation / Flow types are more complex that's an OK tradeoff. Ideally both API and implementation should be simple of course when possible.

I'm not worried about flow types being complex or API implementation. I'm more opposed to the confusing behaviour. It allows users to do strange things which don't make sense, e.g.-

tabBar: (navigation, options) => ({
  focused: true,
})

Can you guess what this does?

I think what @ericvicenti wants is to treat options passed to getScreenConfig the default navigationOptions, but we're not using options that way, and probably default config there won't make sense since we're using getScreenConfig in many places and we can't pass default config everywhere. The default config is already specified as component's default props.

[satya164]

Closing since we decided not to change any current behaviour.

[grabbou]

Reopening as #984 will close it in a bit.