New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

screenIsActive prop / componentDidFocus event for TabNavigator items #51

Closed
JohannesKaufmann opened this Issue Jan 27, 2017 · 208 comments

Comments

Projects
@JohannesKaufmann

JohannesKaufmann commented Jan 27, 2017

In one of my Tabs i need to load Data from the server and do some heavy rendering. I would like to defer that until the Tab is focused in order to improve the JS-Thread performance. How should i implement that?

@browniefed

This comment has been minimized.

browniefed commented Jan 28, 2017

Use componentDidUpdate and compare the active route on navigation.

@JohannesKaufmann

This comment has been minimized.

JohannesKaufmann commented Jan 28, 2017

but how do you get the active route?

@browniefed

This comment has been minimized.

@JohannesKaufmann

This comment has been minimized.

JohannesKaufmann commented Jan 28, 2017

this.props.navigation.state.routeName is the name of the rendered tab even if its not selected. And if you change the tab its not changed.

@JohannesKaufmann

This comment has been minimized.

JohannesKaufmann commented Jan 28, 2017

class SettingsScreen extends React.Component {
  render () {
    console.log('state: ', this.props.navigation.state)
    return (
      <Text>
        {JSON.stringify(this.props, null, 2)}
      </Text>
    )
  }
}

will log:
state: Object {key: "Settings", routeName: "Settings"}
even if the currentTab is Home or Chat

@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented Jan 28, 2017

It probably makes sense to add lifecycle hooks to screens. So on your screen component you could declare:

  • componentWillFocus - this happens after component did mount, before the screen starts animating in
  • componentDidFocus - screen is done animating in
  • componentWillBlur - screen is about to animate out
  • componentDidBlur - screen is done animating out

What do people think about that?

@satya164

This comment has been minimized.

Collaborator

satya164 commented Jan 28, 2017

@ericvicenti sounds good, though we need to think about it a bit more. this is probably clear for stack navigators, but

  • what will happen for navigators that don't do animation?
  • what if componentWillFocus fires, but componentDidFocus didn't (can happen for tabview when you're swiping fast), then should componentWillBlur fire?
@satya164

This comment has been minimized.

Collaborator

satya164 commented Jan 28, 2017

@JohannesKaufmann have you checked the tab navigator docs? there's an option called lazyLoad which does exactly what you want https://reactnavigation.org/docs/navigators/tab

@browniefed

This comment has been minimized.

browniefed commented Jan 28, 2017

We need to pass props to the TabView but we don't or has that changed?

@satya164

This comment has been minimized.

Collaborator

satya164 commented Jan 28, 2017

@browniefed nope. I think we need to discuss it first, since it'd be weird to have some props passed when rendering, and some props passed when creating the navigator. I'll open an issue regarding that.

@hilkeheremans

This comment has been minimized.

Contributor

hilkeheremans commented Jan 28, 2017

I'd like just to chime in here coming from extensive usage of React Router v4 where they explicitly backed away from lifecycle hooks. If we can at all avoid adding lifecycle hooks, I'm all for it. These lifecycle hooks add complexity and, IMHO, are not part of React's basic philosophy. Not that my opinion really matters ;-)

Why not use what we already have -- props to signal a component's current state? Can't the usual lifecycle hooks cover most or all cases at that point? I might be missing some stuff here.

@satya164 can you show me a quick example or provide more detail of the creating vs rendering concern here?

@JohannesKaufmann

This comment has been minimized.

JohannesKaufmann commented Jan 28, 2017

lazyLoad slows down the animation a lot and you can't just activate it for one tab. And I would rather show a View with the correct background colour and a ActivityIndicator while the user is not viewing the tab so that the animation is smooth.

What about a higher order component that gives you more information about the current routing state? Or maybe some way to register to the Navigation Dispatch that you see in the console.

@satya164

This comment has been minimized.

Collaborator

satya164 commented Jan 28, 2017

lazyLoad slows down the animation a lot

It won't if you don't to expensive work during the animation

componentDidMount() {
  InteractionManager.runAfterInteractions(() => this.setState({ ready: true });
}

render() {
  if (!this.state.ready) {
    return <PlaceholderWithIndicator />;
  }

  ...
}
@satya164

This comment has been minimized.

Collaborator

satya164 commented Jan 28, 2017

@satya164 can you show me a quick example or provide more detail of the creating vs rendering concern here?

Which concern are you talking about?

@JohannesKaufmann

This comment has been minimized.

JohannesKaufmann commented Jan 28, 2017

with lazyLoad: true
navigation

The first time i access the Tab Game it takes quite some time for the animation (i assume the animation starts as soon as the component is rendered?). The second time its fast as usual.

@satya164

This comment has been minimized.

Collaborator

satya164 commented Jan 28, 2017

The first time i access the Tab Game it takes quite some time for the animation

Pretty sure this won't be noticeable when you're in production mode

@JohannesKaufmann

This comment has been minimized.

JohannesKaufmann commented Jan 28, 2017

@satya164 In __DEV__ = false mode its still noticeable so lazyLoad: true is not an option. (but I haven't tried building for production)

As I'm currently using wix/react-native-navigation with smooth animations this is something that is super important for me...

@satya164

This comment has been minimized.

Collaborator

satya164 commented Jan 28, 2017

If it's noticeable in production mode, then it's something we need to fix.

As I'm currently using wix/react-native-navigation with smooth animations this is something that is super important for me...

Animation performance is sure important, regardless of which navigator you use. We use native animations where we can, so animation performance should be good, if you see animation is janky somewhere, please file an issue. Remember that this library is still in beta.

@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented Jan 28, 2017

@hilkeheremans I do appreciate that lifecycle hooks should be used sparingly. We got this far and we haven't needed them, but there are often things in real-world apps that demand this use case. Just like most things with react, the declarative API is what you almost always want to use, but occasionally, imperative lifecycle hooks are needed. One of the greatest things about react is that it gives you a great declarative interface, but it also provides the component lifecycle methods for times when the declarative API is insufficient and you need an "escape hatch" to imperative programming. The handling of focus is one of these examples.

@satya164, the easiest way to handle the "viewWillAppear race condition is to skip that hook and only provide viewDidAppear and viewDidDisappear

@satya164

This comment has been minimized.

Collaborator

satya164 commented Jan 28, 2017

@ericvicenti I think those might be useful. since the only thing wrong about them is the names don't match what it does, how about,

componentWillAnimateIn
componentWillAnimateOut
componentDidFocus
componentDidBlur

@satya164

This comment has been minimized.

Collaborator

satya164 commented Jan 30, 2017

Also, in addition to these hooks, we can add an HOC withNavigationFocus or something, which uses these hooks to pass a focused prop to the component, so that the components can use it declaratively and don't need to use the imperative API.

@ivpusic

This comment has been minimized.

ivpusic commented Jan 31, 2017

@satya164 do you have idea when something like this will be implemented?

@satya164

This comment has been minimized.

Collaborator

satya164 commented Jan 31, 2017

@ivpusic feel free to send a PR to implement it

@satya164

This comment has been minimized.

Collaborator

satya164 commented Feb 9, 2017

I think following will be a better API,

navigation.addListener('animateIn', doStuff);
navigation.addListener('animateOut', doStuff);
navigation.addListener('focus', doStuff);
navigation.addListener('blur', doStuff);

Lifecycle hooks will break when using HOCs and you always need to be the direct child of the navigator to use the lifecycle hooks.

Events will work everywhere, even inside the components of navigationOptions, and with withNavigation HOC.

@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented Feb 9, 2017

Unfortunately I think this API won't be very easy to use, because you'll need to re-subscribe every time the navigation prop changes, by listening to lifecycle hooks. Also you'll need to unsubscribe.

So I'm afraid the developer experience is worse this way, but I can still be convinced otherwise. Maybe you can post both examples so we can see them side-by-side?

I do agree the HOC problem is real, but I think there are ways to work around it.

@hilkeheremans

This comment has been minimized.

Contributor

hilkeheremans commented Feb 9, 2017

I'm not too convinced about the pure listener approach either. I like what @satya164 mentioned before: what if we implement BOTH the good old imperative AND a declarative approach, ie:

// lifecycle methods
componentWillAnimateIn
componentWillAnimateOut
componentDidFocus
componentDidBlur

// props on navigation (possible props:)
focused
isFocusing
isBlurring

This would alleviate the HOC issue raised above, wouldn't it? Not saying this is a great idea, just throwing it in here to get to better solutions.

@browniefed

This comment has been minimized.

browniefed commented Feb 9, 2017

I believe there was conversation at some point but is there a potential to just use React lifecycle methods to accomplish all of these vs having to explicitly add this stuff?

@satya164

This comment has been minimized.

Collaborator

satya164 commented Feb 9, 2017

because you'll need to re-subscribe every time the navigation prop changes

I believe this can be easily solved. it's only a problem if the navigation prop keeps the listeners. But if the navigator keeps the listeners, then this won't be a problem. Even with lifecycle methods, the navigator needs to store a ref, so it's not much different.

So I'm afraid the developer experience is worse this way, but I can still be convinced otherwise.

It's just a little bit worse because you always have to explicitly un-subscribe. With life-cycle hooks, the developer experience is much worse when you're using HOCs such as Redux's connect. In most of my apps, most of the screens use connect.

Though the worse dev experience in events approach is easy to solve using a HOC we'll provide, which manages the subscription under the hood.

Personally I think most people should use withNavigationFocus HOC we provide instead of using this API directly. This API will act as a low-level API, and I think events provide much more flexibility than lifecycle hooks, which is nice for a low-level API. Also it's very easy to build a life-cycle hooks API with it if someone really wants it.

his would alleviate the HOC issue raised above

We cannot provide a focused prop to all screens because it'll trigger a re-render every-time it changes. Components should explicitly tell us that they want this prop. I think withNavigationFocus HOC is the way to go.

Lifecycle Hooks

Pros

  • Easy to use, no need to manually subscribe and unsubscribe
  • Can use withNavigationFocus HOC to wrap which passes a focused prop to get a declarative API

Cons

  • Won't work when your screen is wrapped in HOC, can use withNavigationFocus HOC to workaround
  • If a child component wants to know about focus state, you've to pass it down, re-rendering everything when state changes
  • withNavigationFocus HOC only works for the screen component (we can do extra work to make it work for components down the tree by implementing a subscription system internally)

Events

Pros

  • Works fine with HOCs
  • Can pass down to child in tree without worrying about re-render, or use withNavigationFocus HOC deep in tree to know when screen is focused
  • Can use HOC to wrap the component which passes a focused prop, which solves manual subscription woes, and get a declarative API
  • withNavigationFocus HOC works for any component down the tree
  • We can automatically cleanup listeners if the screen unmounts, or warn the developer
  • A small DX improvement is that flow will work with it

Cons

  • Developers have to explicitly subscribe and unsubscribe from Events, can use HOC to avoid manual work
@bramus

This comment has been minimized.

bramus commented Dec 8, 2017

Hah, clever one @easydaniel :)

@dustturtle

This comment has been minimized.

dustturtle commented Dec 15, 2017

really need a hook~ for us native developers, it is so nature

@JuniorBoaventura

This comment has been minimized.

JuniorBoaventura commented Dec 21, 2017

@easydaniel Do you have a solution for when the user swipes between tabs like on android ?

@punksta

This comment has been minimized.

punksta commented Dec 21, 2017

I have screenIsActive property implementation with redux integration here(https://github.com/punksta/thecatnative). withCurrentRoute is a hoc which modify props based on mapperFn: (PropsInput, ?Route) => PropsOutput function. withActiveRoute is basic usage of it. withActiveRoute adds isActive ussing mapperFn. And then we can write something like this:

class OriginalScreen extends Component {

  shouldComplonentUpdate(nextProps) {
      return nextProps.isActive && anything...
  }
  componentWillReceiveProps(nextProps) {
      const willShow = !this.isActive && nextProps.isActive
      const willHide = this.isActive && !nextProps.isActive
  }
}
const OriginalScreenWithActiveProp = withActiveRoute(OriginalScreen);
const mapToState = state => ({
	nav: state.nav // redux navigation state
});
export const connect(mapToState)(OriginalScreenWithActiveProp)
@easydaniel

This comment has been minimized.

easydaniel commented Dec 21, 2017

@JuniorBoaventura perhaps using redux will be a better solution with binding the nav state and simply do something like

class Screen extends Component {
    componentWillFocus() {
         // Screen will entered
    }

    componentWillBlur() {
         // Screen will leave
    }
    componentWillReceiveProps(nextProps) {
         if (nextProps.nav.state.routeName === CURRENT_SCREEN_ROUTE_NAME') {
              this.componentWillFocus()
         } else if (nextProps.nav.state.routeName !== CURRENT_SCREEN_ROUTE_NAME && this.props.nav.state.routeName === CURRENT_SCREEN_ROUTE_NAME) {
              this.componentWillBlur()
         }
    }

    ...rest of screen stuff
}
export default enhancedScreen = connect(mapStateToProps, mapDispatchToProps)(Screen)
@JuniorBoaventura

This comment has been minimized.

JuniorBoaventura commented Dec 22, 2017

@easydaniel Thank you I was thinking about doing something like this but I ended up using react-navigation-is-focused-hoc that works for tap on the tabbar and the slide gesture without any redux implentation.

@tezqa

This comment has been minimized.

tezqa commented Dec 22, 2017

@punksta Your workaround is perfectly working, thanks!

And sorry to the team, your work is awesome but it's very disappointing that a such component fails to handle basic lifecycle... Sorry for my harsh words, but it should be a basic feature.

@samuela

This comment has been minimized.

samuela commented Jan 8, 2018

First of all let me just say: It is jaw-dropping that this is not already in react-navigation.

Ok, with that out of the way here's my workaround.

An example screen:

import NavigationStateNotifier from './NavigationStateNotifier'

class MyScreen extends Component {
  constructor (props) {
    super(props)
    this.state = {
      // If you'd like to have an `activeScreen` state flag, you can. But you could do anything else you want really!
      activeScreen: false

      // ...
    }

    NavigationStateNotifier.newListener(
      this,
      () => {
        this.setState({activeScreen: true})
        // anything else that you'd like to do when this screen is navigated to
      },
      () => {
        this.setState({activeScreen: false})
        // anything else that you'd like to do when this screen is navigated off of
      }
    )
  }

  render () { /* ... */ }
}

Here's App.js:

import NavigationStateNotifier from './app/NavigationStateNotifier'

const AppNavigator = StackNavigator({
  Home: { screen: MyScreen },
  // and so on...
})

export default class extends React.PureComponent {
  render () {
    return (
      <AppNavigator
        onNavigationStateChange={(prevState, currentState) =>
          NavigationStateNotifier.onNavigationStateChange(prevState, currentState)}
      />
    )
  }
}

And finally here's the good stuff, 'NavigationStateNotifier.js`:

class Listener {
  constructor (notifier, key, onNavEnter, onNavExit) {
    this.notifier = notifier
    this.key = key
    this.onNavEnter = onNavEnter
    this.onNavExit = onNavExit
  }

  off () {
    this.notifier.removeListener(this)
  }
}

class Notifier {
  constructor () {
    this.listeners = new Set()
  }

  removeListener (listener) {
    this.listeners.delete(listener)
  }

  onNavigationStateChange (prevState, currState) {
    const prevKey = prevState.routes[prevState.index].key
    const currKey = currState.routes[currState.index].key

    // Handle all exit events...
    this.listeners.forEach((listener) => {
      if (listener.key === prevKey) {
        listener.onNavExit()
      }
    })

    // ...then all enter events
    this.listeners.forEach((listener) => {
      if (listener.key === currKey) {
        listener.onNavEnter()
      }
    })
  }

  newListener (screen, onNavEnter, onNavExit) {
    const listener = new Listener(
      this,
      screen.props.navigation.state.key,
      onNavEnter,
      onNavExit
    )
    this.listeners.add(listener)
    return listener
  }
}

export default (new Notifier())
@elyran

This comment has been minimized.

elyran commented Jan 16, 2018

Indeed surprising this feature is missing. So basic!

Thanks to the team anyway 😄

Here's my solution, I tried keeping it as simple as possible:

App.js

import EventEmitter from 'EventEmitter'

class App extends Component<{}> {
  render() {
    const Tabs = TabNavigator({
      Home: {
        screen: HomeScreen,
      },
      Bookmarks: {
        screen: BookmarksScreen,
      },
    })

    const navigationEvents = new EventEmitter()

    return <Tabs
      screenProps={{ navigationEvents: navigationEvents }}
      onNavigationStateChange={(prevState, newState, action) => {
        if('Navigation/NAVIGATE' == action['type'])
          navigationEvents.emit(`onFocus:${action['routeName']}`)
      }}
    />
  }
}

Bookmarks.js

const ROUTE_NAME = 'Bookmarks'

class ToExport extends Component<{}> {
  constructor() {
    super()

    this.onFocus = this.onFocus.bind(this)

    this.state = {
      //
    }
  }

  componentDidMount() {
    this.props['screenProps'].navigationEvents.addListener(`onFocus:${ROUTE_NAME}`, this.onFocus)
  }

  componentWillUnmount() {
    this.props['screenProps'].navigationEvents.removeListener(`onFocus:${ROUTE_NAME}`, this.onFocus)
  }
}

Very simple.
It would be even simpler if the component had this support:

  • I could pass a screenProps per screen rather than a general one.
  • I could tell my own screen name from within Bookmarks.
@Gardezi1

This comment has been minimized.

Gardezi1 commented Jan 18, 2018

any update on this ?

@Gardezi1

This comment has been minimized.

Gardezi1 commented Jan 19, 2018

@elyran I'm trying to use your solution I can't find anything with key route name etc

this is what I'm getting in action

Object {
  "key": "Init-id-1516382760253-1",
  "params": Object {
    "title": "Shopping Cart",
  },
  "type": "Navigation/SET_PARAMS",
}
@victorbadila

This comment has been minimized.

victorbadila commented Jan 19, 2018

Edited: sorry, too early in the morning

I have tried @elyran 's solution and it worked for me with little modifications. I was able to read the route name though not from the action, but from the previous / next state.
Something like:

<Navigator onNavigationStateChange={(prevState, newState) => {
        const lastRoute = newState.routes[0];
        if (lastRoute.routeName === 'MyTabNavRoute') {
          const actualRoute = lastRoute.routes[lastRoute.index].routeName;
          if (actualRoute === 'CertainTab') {
            console.log('navigate to CertainTab, emit events or do things here');
          }
          if (actualRoute === 'OtherTab') {
            console.log('navigate to OtherTab, do other things here');
          }
        }
@elyran

This comment has been minimized.

elyran commented Jan 19, 2018

@Gardezi1

This comment has been minimized.

Gardezi1 commented Jan 19, 2018

@victorbadila

I've alot of stack navigators inside a tab navigator. On a particular stack navigator I want such kind of functionality. Yeah I looked into it and found I was looking at the wrong parameter. I had to look at prev and new one.

the problem is it is not triggering. Can you look at the code and tell me what is it that I'm doing wrong. I think I'm not attaching the even properly but I can't figure out what is it that I'm doing wrong

navigation/index.js

CartScreen: {
      screen: ({navigation}) => <CartScreenStack
          screenProps={{rootNavigation: navigation, navigationEvents: navigationEvents}}
          onNavigationStateChange={(prevState, newState, action) => {
             
              const lastRoute = newState.routes[0];
              if (lastRoute.routeName === 'Cart') {
                 

                  navigationEvents.emit(`onFocus:Cart`)
                  navigationEvents.emit(`onBlur:Cart`)
              }
          }}
      />,

cartScreen/index.js

componentWillMount() {
        // this.props.navigation.setParams({title: Languages.ShoppingCart});

        this.props.screenProps.navigationEvents.addListener('onFocus:Cart', this.onFocus)
        this.props.screenProps.navigationEvents.addListener('onBlur:Cart', this.onBlur)
  }

  onFocus = () => {
    console.log('it is in focus again');
  }
  onBlur = () => {
    console.log('it is in blur again');
  }

@zeflash

This comment has been minimized.

zeflash commented Jan 22, 2018

I usually don't pollute threads but in this case, I need to say something :)

This thread is bad for everyone - It's the first result from google when searching for focus / react navigation, and in it you mainly get discussion about pros/cons, hacks & patches with a very wide context (using redux or not, using props or not, etc). But no clear choice and resolution from the react-navigation team.
It's therefore more misleading than helpful.

Most of it is a discussion on how to implement a fix to the issue and therefore should be taken elsewhere.
Mainly like other said, the problem is that it's been a year without an actual resolution from the owners. And no one can be expected to go through a year of "me too" or we should do it imperative or reactive and pros & cons. In a way this thread has reached its critical mass: people are able to tell this thread clearly relates to their issues, they look for the first comments in which most of the potential solutions are laid out, and then either +1, or add another comment stating this or that solution should be implemented. And it can keep growing forever like this.

The actual solutions - cause they exist - are lost in the middle of this mess. They should be catalogued and put in some section of the readme / help - to actually help people & not make them loose hours on this!
In my case, react-navigation-is-focused-hoc Kudos to him for this HOC.

@Gardezi1

This comment has been minimized.

Gardezi1 commented Jan 22, 2018

@zeflash is there any for onBlur ??

@zeflash

This comment has been minimized.

zeflash commented Jan 22, 2018

@Gardezi1 no idea - I ended up going the react way. My limited knowledge would tell you that callbacks like this can't properly be achieved via external means and need to be done in the actual code, which isn't done atm.

You can always go through the thread to figure it out though ;)

@mgscreativa

This comment has been minimized.

mgscreativa commented Jan 23, 2018

Hi! After some digging I was told that development of this feature is taking place in this branch https://github.com/react-navigation/react-navigation/tree/%40evv/navigation-events and also was told it has an ETA of several days...

Regarding this does @ericvicenti, @skevy or @brentvatne have a more concise ETA for this?

Thanks!

@brentvatne

This comment has been minimized.

Member

brentvatne commented Jan 23, 2018

@mgscreativa - a PR for this will be open soon. you can read the RFC for more info: https://github.com/react-navigation/rfcs/blob/master/text/0001-event-system.md

closing because we're keeping issues for bugs now and new features belong in either the rfc repo or https://react-navigation.canny.io/feature-requests

@kelset

This comment has been minimized.

Contributor

kelset commented Jan 24, 2018

Quick update for everyone, the PR to implement this is now live: #3345

So if you want to review/comment on that implementation, now you can.

@gtaylor44

This comment has been minimized.

gtaylor44 commented Jan 24, 2018

My email inbox is flooded with notifications from this thread. Glad an official feature might finally be on the way.

@react-navigation react-navigation locked as resolved and limited conversation to collaborators Jan 24, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.