slorber commented 5 years ago

Hi,

Today, when we add a will/didFocus listener on a screen, if that screen is currently focused, the listener will fire.

I think that behavior was implemented at a time where navigation.isFocused() and withNavigationFocus didn't exist yet, does not make sense anymore and is complicating the library usage.

For example, it makes it impossible to unsubscribe/resubscribe the same listener (or eventually closure/arrow function) without any side effect, which complicates the implementation of consuming code which need to ensure to register "stable listeners"

Also, one major usecase for navigation events is to refresh the screen data on focus. The problem is that we use data loading systems that may already perform a fetch on mount (react-apollo, react-refetch...), so wiring onWillFocus={() => refetch()} will actually trigger a duplicate fetch on mount. As mentionned during the implementation of <NavigationEvents /> here, I solved this problem on my app with this kind of code:

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++;
    }
  })();

Removing the willFocus event after subscribe will solve that problem for all people that are trying to refetch on screen focus. If the behavior is intended it can still added back in componentDidMount anyway.

This is a breaking change and users should perform these migrations:

1) If user wants to fire something on mount if the screen is focused, he can implement that with navigation.isFocused() in componentDidMount

2) If user wants to track focused state, he can use withNavigationFocus (also implemented later) or init his state with navigation.isFocused()

If this is validated I'd be happy to send a PR to all the concerned projects of the system (core, NavigationEvents, hooks)

brentvatne commented 5 years ago

Today, when we add a will/didFocus listener on a screen, if that screen is currently focused, the listener will fire.

can you provide a snack that demonstrates clearly what is happening here?

slorber commented 5 years ago

Hi @brentvatne

TLDR

I was not totally correct in all my assumptions above. The following seems to apply to willFocus and didFocus

Infinite loop.

When using declarative event comp or hooks, it's simpler to unsub/resub, but this can easily lead to the following:

  componentDidMount() {
    this.props.navigation.addListener('willFocus', () => {
      this.incrementCounter();
      this.props.navigation.addListener('willFocus', () => {
        this.incrementCounter();
      });
    });
  }

Counter = 2, but I think in such case it should be counter = 1, which would solve the infinite loop problem. Redux had a similar issue too.

Breaking change would be small and may be considered as a bug.

Do we really want willFocus on mount?

When mounting the initial navigation, on the initial route names, without any animation etc, do we really want focus events to fire? What are the usecases for this? This complicates the usecase where you want to refresh api data on focus, because it might lead to a double fetch on mount.

More important breaking change, but with clear migration path possible.

Details

Infinite loop.

Here is a snack that shows the infinite loop problem the current solution can actually trigger: https://snack.expo.io/@slorber/focus-event

The original issue reporting this is https://github.com/react-navigation/react-navigation/issues/5058

This loop happens because:

The listener triggers a state change
We unsub/resub on every update
The resub triggers the listener

My snack may feel convoluted and the unsub/resub unnecessary, but when using declarative approachs like or hooks, particularly when providing closures as listeners, it's important to ensure the last closure provided is run because a closure capture external variables at creating time, and those variables may change over time. Unsub/resub permits to easily ensure the last closure is run.

Actually I think my initial description of the problem is not totally correct. It seems unsub/resub happening inside a "didFocus" is actually the problem. The button in the snack does show that when the screen is already focused, and we add a listener, this listener will actually not be fired even if the screen is focused (where I was wrong).

As far as I remember, Redux had this issue too and solved it by creating a listener array copy before iterating on them, so that on resub, the newly added listener won't be executed before next focus. Doing something similar can make sense here to ensure that we don't encounter any infinite loop.

The following should only print 1 imho, currently it prints 2: https://snack.expo.io/@slorber/focus-event-2

Do we really want willFocus on mount?

This would solve the infinite loop issue, but would not solve the issue where the user might not necessarily want this listener to fire on mount (because it may be wired to a "refresh api data" method)

For me the main usecase to willFocus was mostly to track focused screen before, but we have better alternatives now (withNavigationFocus, navigation.isFocused()...). The main usecase that remains for me is to refresh api data on screen focus. But generally the tools we use already do the fetching on mount (like Apollo) so having this event firing will trigger a "double fetch" on mount. Do we really want this event to fire when the initial navigator screen is mounting?

I don't know how users are using this listener, maybe my assumptions are incorrect, and it would be a larger breaking change.

react-navigation / rfcs

On will/didFocus subscribe, stop firing the listener if current screen is focused #72

TLDR

Details