Declarative way to register event listener (<OnNavigationEvent/>) about rfcs

[satya164]

Looks good!

Not sure the test in componentDidUpdate is really required as deleting/adding to the listener set is fast,

Guess it doesn't hurt to keep it.

we want to preserve listener order

Probably unnecessary.

[ericvicenti]

The purist in me wants to say that those sort of components are improper because they aren't actual views that appear on the screen.

But I've worked with codebases that do this heavily, and the developer ergonomics are pretty fantastic.

For us, it might look like this:

<NavigationEvents
  onWillFocus={...}
  onDidFocus={...}
  onWillBlur={...}
  onDidBlur={...}
/>

cc @satya164 who has also proposed componenty things like this.

[satya164]

Yeah, components for events is pretty nice. With React.Fragment they are even better since we don't need a wrapper View anymore!

If we want to provide such a component, the one @ericvicenti suggested looks nice! It can get navigation from the context.

[slorber]

Great. Also not fan of using components everywhere but yes it's often convenient No need for view rendering null will also work :) Will make a WIP PR soon so you can review. Le ven. 4 mai 2018 à 13:10, Satyajit Sahoo <[email protected]> a écrit :

…

Yeah, components for events is pretty nice. With React.Fragment they are even better since we don't need a wrapper View anymore! If we want to provide such a component, the one @ericvicenti <https://github.com/ericvicenti> suggested looks nice! It can get navigation from the context. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtvPum15NydMv55FNF1KVk68IlrwyXVks5tvDctgaJpZM4TuAbg> .

[slorber]

Here is a first draft implementation.

Note that I used an "internal listener" for each event, because we don't want to unsubscribe/resubscribe the real listener on every update, as the user would very likely provide an arrow function like this:

<NavigationEvents onWillFocus={() => doSomething()}/>

Yet, we probably want to support this case where the behavior of the provided listener is updated at runtime:

<NavigationEvents onWillFocus={isEnabled ? doSomething : doSomethingElse}/>

Please tell me what you think and if I should make a PR with such implementation.

import React from 'react';
import withNavigation from './withNavigation';

const EventNameToPropName = {
  willFocus: 'onWillFocus',
  didFocus: 'onDidFocus',
  willBlur: 'onWillBlur',
  onDidBlur: 'onDidBlur',
};

const EventNames = Object.keys(EventNameToPropName);

class NavigationEvents extends React.Component {
  constructor() {
    super();
    this.subscriptions = {};
    this.updateListeners();
  }

  componentDidUpdate() {
    this.updateListeners();
  }

  componentWillUnmount() {
    EventNames.forEach(this.removeListener);
  }

  updateListeners = () => {
    EventNames.forEach(eventName => {
      const propName = EventNameToPropName[eventName];
      if (this.props[propName] && !this.subscriptions[eventName]) {
        this.setupListener(eventName);
      }
      if (!this.prop[propName] && this.subscriptions[eventName]) {
        this.removeListener(eventName);
      }
    });
  };

  setupListener = eventName => {
    const propName = EventNameToPropName[eventName];
    let count = 0;
    const listener = payload => {
      // /!\ It's important to get ref to the callback inside the closure to handle callback updates
      this.props[propName](payload, count);
      count++;
    };
    this.subscriptions[eventName] = this.props.navigation.addListener(
      eventName,
      listener
    );
  };

  removeListener = eventName => {
    this.subscriptions[eventName] && this.subscriptions[eventName].remove();
  };

  render() {
    return null;
  }
}

export default withNavigation(NavigationEvents);

Note that in the callback, I provide the payload and a count. I'm not really sure of the API of this callback, maybe you have opinion on that?

What I can say is that in my app, I need the infos provided (count + payload) to implement the behavior I want: refresh a Tab on every focus, but not on initial mount.

        <NavigationEvents
          onWillFocus={(payload,count) => {
            if ( payload.action.type === NavigationActions.BACK ) {
              return; // We don't want to refresh on back so that user does not loose pagination state
            }
            if ( count > 0 ) {
              this.refreshTabData()
            }
          }}
        />

Any opinion on the callback API design?

[slorber]

I think the counter I need above can be implemented in userland instead.

Even if it's less handy for my usecase, it would probably pollute less the api

class MyClass extends React.Component {

  handleWillFocus = (() => {
    let count = 0;
    return payload => {
            if ( payload.action.type === NavigationActions.BACK ) {
              return; // We don't want to refresh on back so that user does not loose pagination state
            }
            if ( count > 0 ) {
              this.refreshTabData()
            }
           count++;
    }
  })();

  render() {
    return (
      <NavigationEvents onWillFocus={this.handleWillFocus}/>
    )
  }
}

[satya164]

In your NavigationEvents component, you're adding the listeners in constructor. AFAIK, with async rendering in react, it's not gurranteed that component will mount when constructor/willMount is called and hence willUnmount won't be called to clean up the listener. The listener might end up getting added multiple times causing a memory leak.

Note that I used an "internal listener" for each event, because we don't want to unsubscribe/resubscribe the real listener on every update

Is there any difference between re-subscribing to internal listener and real listener? In theory, resubscribing to the real listener should be of the same impact.

I provide the payload and a count

I think it's unncecessary to provide a count.

[slorber]

In your NavigationEvents component, you're adding the listeners in constructor. AFAIK, with async rendering in react, it's not gurranteed that component will mount when constructor/willMount is called and hence willUnmount won't be called to clean up the listener. The listener might end up getting added multiple times causing a memory leak.

So you think it's better to add listeners in componentDidMount?

Is there any difference between re-subscribing to internal listener and real listener? In theory, resubscribing to the real listener should be of the same impact.

In theory, if the cost of removing/adding listener is not significant, and if I keep the same callback API, I could register directly the prop functions as listeners and make a simpler implementation.

Does anyone see any performance issue? I think it would be ok as it would only be a Set remove/add op: https://github.com/react-navigation/react-navigation/blob/8ed3817c9030f4b790f0e24e0197d8dde9b724d1/src/getChildEventSubscriber.js#L149

[satya164]

So you think it's better to add listeners in componentDidMount?

Yeah, that's the recommended way.

[slorber]

Does this look good to you?

import React from 'react';
import withNavigation from './withNavigation';

const EventNameToPropName = {
  willFocus: 'onWillFocus',
  didFocus: 'onDidFocus',
  willBlur: 'onWillBlur',
  onDidBlur: 'onDidBlur',
};

const EventNames = Object.keys(EventNameToPropName);

class NavigationEvents extends React.Component {
  constructor() {
    super();
  }

  componentDidMount() {
    this.subscriptions = {};
    EventNames.forEach(this.addListener);
  }

  componentDidUpdate(prevProps) {
    EventNames.forEach(eventName => {
      const listenerHasChanged =
        this.props[EventNameToPropName[eventName]] !==
        prevProps[EventNameToPropName[eventName]];
      if (listenerHasChanged) {
        this.removeListener(eventName);
        this.addListener(eventName);
      }
    });
  }

  componentWillUnmount() {
    EventNames.forEach(this.removeListener);
  }

  addListener = eventName => {
    const listener = this.props[EventNameToPropName[eventName]];
    if (listener) {
      this.subscriptions[eventName] = this.props.navigation.addListener(
        eventName,
        listener
      );
    }
  };

  removeListener = eventName => {
    if (this.subscriptions[eventName]) {
      this.subscriptions[eventName].remove();
      this.subscriptions[eventName] = undefined;
    }
  };

  render() {
    return null;
  }
}

export default withNavigation(NavigationEvents);

Not sure the test in componentDidUpdate is really required as deleting/adding to the listener set is fast, but maybe if there are many listeners in the set we want to preserve listener order? (this order will not be kept with arrow functions anyway so this may be quite useless?)

[slorber]

closing as it just got merged ! :)