Closed satya164 closed 4 years ago
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 😟
@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.
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
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.
@serhiipalash - why couldn't you just rewrite the firebaseConnect
decorator to grab the userId from context?
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.
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>
}
}
you can just create another HOC
yes, I was going to do like this, when screenProps will be depricated
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.
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.
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.
The only thing preventing the removal of screenProps
is that you cannot access context in navigationOptions
, which should be addressed by #24
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:
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.mode: modal
navigator used for full-screen dialogs.headerStyles
, header tint, drawerLabel
, or drawerIcon
using a dynamic context based theme.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.
I very much agree with this! It's workaround that is better served by using React features