Deprecate screenProps

[satya164]

No description provided.

[brentvatne]

I very much agree with this! It's workaround that is better served by using React features

[serhiipalash]

Hi!
I am not agree with this. Please leave screenProps api at least for primitives. If you do this, it will break all our app architecture.
We use react-redux-firebase HOC and it is very easy to use it if you have userId in the props for each screen, otherwise we will have >10 addition lines of code.

RootNavigator

<RootNavigator
  navigation={addNavigationHelpers({
    dispatch: this.props.dispatch,
    state: this.props.router,
  })}
  screenProps={{
    userId: this.props.user.id,
  }}
/>

Screen

import { connect } from 'react-redux'
import { firebaseConnect } from 'react-redux-firebase'

@firebaseConnect(props => [`todos/${props.userId}`])
@connect(state => ({ todos: state.firebase.todos }))
class TodosListScreen extends React.Component {
    render() {
        const { todos } = this.props

        return <div>{todos.map(item => <span key={item.id}>{item.name}</span>)}</div>
    }
}

Image that instead of this clean and neat code we will have to declare contextTypes then add componentWillMount method and write data fetching logic there 😟

[satya164]

@serhiipalash The way you are using screenProps deoptimizes all shouldComponentUpdate down the tree. Which is exactly one of the reason I'm proposing the removal.

If you're concerned with lines of code, you can write your own HOC to address that. Also, it's a state management problem. There is no reason state management solutions should be in a navigation library.

[serhiipalash]

The way you are using screenProps deoptimizes all shouldComponentUpdate down the tree.

np, we can do like this

import memoize from 'fast-memoize'

class App extends React.component {
  render() {
    return (
      <RootNavigator
        navigation={addNavigationHelpers({
          dispatch: this.props.dispatch,
          state: this.props.router,
        })}
        screenProps={this._getScreenProps()}
      />
    )
  }

  _getScreenProps = () => this._getMemoizedScreenProps(this.props.user.id)

  _getMemoizedScreenProps = memoize(userId => ({ userId }))
}

But I understand your arguments that state management is not navigation library problem.

Still it would be great if you kept screenProps api

[satya164]

You can do that, but it's too easy to forget and most of the code I've seen never do it. And even if you do that, whenever the screen props changes, all the screens in the tree are gonna re-render, even if only a few actually use it, not a big deal, but unnecessary. It avoids a little bit of code, yes, but I think being explicit here avoids all the issues associated with this and makes you handle it in a proper way, i.e. with redux, react context or another state management solution. There are other advantages of using context, you can access it in components which aren't screens without having to pass it down manually.

[brentvatne]

@serhiipalash - why couldn't you just rewrite the firebaseConnect decorator to grab the userId from context?

[serhiipalash]

why couldn't you just rewrite the firebaseConnect decorator to grab the userId from context?

firebaseConnect is taken from react-redux-firebase library.

Of course I can rewrite it, but it would be the same as I rewrote connect from react-redux library. Not good idea.

[satya164]

You don't have to rewrite it, you can just create another HOC which gets the user id from context and passes it as a prop.

@userIdFromContext
@firebaseConnect(props => [`todos/${props.userId}`])
@connect(state => ({ todos: state.firebase.todos }))
class TodosListScreen extends React.Component {
    render() {
        const { todos } = this.props

        return <div>{todos.map(item => <span key={item.id}>{item.name}</span>)}</div>
    }
}

[serhiipalash]

you can just create another HOC

yes, I was going to do like this, when screenProps will be depricated

[serhiipalash]

But still,
I understand that screenProps cause misunderstandings and their incorrect usage can downgrade app performance, and @satya164 the opinion is correct from the standpoint of library development,
but from the position of app architecture development it's great if you can share some values across the entire application, for example locale or theme.
Should theme change trigger rerendering of the entire app? If it should, how can I do this with react-navigation?
Right now the only way (as far as I know) is to update screenProps on theme change, it will trigger rerendering of the every screen.
Or I can manually add something like @themeConnect on top of each of my screens (>50 right now). In this way there is no sense to use <ThemeProvider>...</ThemeProvider> as my app wrapper.

[dantman]

If you need global state data in every single route I don't see anything wrong with expecting that every one of those routes will have a HOC to get that data. React Navigation is a navigation library, it shouldn't really be in charge of passing global state. Also honestly, for the specific theme case, depending on your routes it may make more sense for your UI components to be the ones using @themeConnect; not your routes, which is something that screenProps cannot help you with.

I suppose the only reasonable counter-argument to this is that direct prop passing is the typical React way, not context which is an advanced extra, and React Navigation is in the way of the App container from directly passing props to the route components within. But even then rather than being the direct passing props to routes, screenProps work more like a knockoff context.

[ericvicenti]

I would be so excited to kill screenProps. I think a lot of people use abuse it to pass data to their screens, but this usage usually causes re-renders of the whole app, and then new users blame ReactNav for poor performance. If we discourage use of screenProps, I think people will find another way to pass data that will not slow down their whole app.

[satya164]

The only thing preventing the removal of screenProps is that you cannot access context in navigationOptions, which should be addressed by #24

[dantman]

Oh, I just though of a use case that isn't handled by #5 and #24 we forgot.

We still have a static navigationOptions (or defaultChildNavigationOptions after #26). It's possible that these too may require context, and they aren't solved by the per-screen options in #24 because this is an attempt to set the same options pattern for all routes in a router, not just specific screen components.

Although perhaps the solution may be to think of all the possible use cases for default navigation options, then come up with alternative router config options that cover those use cases, and deprecate most of the static navigationOptions. (Then we'd just have navigation config and dynamic options in render()).

Some I can think of off the top of my head:

• What if you're trying to use headerLeft to customize the back button or add a drawer's menu button to the components in a StackNavigator nested in. Now what if these customizations had text (eg: An iOS style text based back button or accessibility labels for "Back" or "Menu") and you wanted to localize them with a context/redux based i18n system.
  • Personally I'm moving away from doing it this way. I'm planning to instead do this using a custom Header. Although I'm finding trying to override the back button within a Header ends up a bit like a hack. Perhaps we could solve this by improving the API for extending the Header, adding a header config option to change the Header component used by a StackNavigator (instead of this only being a per-route config), recommending people use it.
  • Not just overriding the style of the back button, this is also applicable to replacing it with a close button for a mode: modal navigator used for full-screen dialogs.
• What if you're customizing headerStyles, header tint, drawerLabel, or drawerIcon using a dynamic context based theme.

[satya164]

Closing since screenProps was removed in React Navigation 5. With its component based API, all problems we discussed here aren't relevant for v5 and React context works perfectly for the use cases.