Memoize navigation action functions

[slorber]

Related to this hooks issue: react-navigation/hooks#3 (comment)

People are trying to use the navigate() function in useEffect, and try to comply to eslint dependency rules, unfortunately the navigation function provided by the NavigationContext changes often, and lead to unwanted effect re-executions on each screen change.

function ReactComponent() {
  const {navigate} = useNavigation();

  useEffect(() => {
      if (user) {
          navigate(...)
      }
  }, [user, navigate])

 return null
}

Not sure exactly how to fix this, but it would be nice if the navigation actions would be memoized to avoid being recreated.

Could it make sense to cache per route key the result of getNavigationActionCreators / router.getActionCreators ?

https://github.com/react-navigation/core/blob/e514a61f9314e8d771231de9d3a4ebf64f661003/src/getNavigation.js#L46
https://github.com/react-navigation/core/blob/e514a61f9314e8d771231de9d3a4ebf64f661003/src/routers/getNavigationActionCreators.js

[satya164]

It's supposed to be memoized, but there seems to be a bug: https://github.com/react-navigation/core/blob/e514a61f9314e8d771231de9d3a4ebf64f661003/src/getChildrenNavigationCache.js

We definitely need to fix this. I'll take a look at it after RNEU. But if you have time to check it, it'll be great too.

Though this code doesn't feel right to me. I assume they have a login screen or something and they want to navigate when the user becomes available after a login. But the code means "run this effect based on user object", so the effect is going to run whenever user object changes which isn't probably what they intend.

Correct code would be something like this (which also wouldn't be problematic due to navigation changing too often):

useEffect(() => {
  if (prevUserRef.current !== undefined && user !== undefined) {
    navigate(...)
  }

  prevUserRef.current = user;
}, [user, navigate])

IMO if the code is behaving unexpectedly due to a dep changing, then there's an actual bug with the code which needs to be fixed. Caching navigation object is purely a perf optimization and shouldn't impact behaviour of the app. The dependency array should also be treated as a perf optimization.

[slorber]

Thanks for the link, didn't know about this caching system.

Asked the user what's his intent with this code. Not sure what's the intent of your code either :D If you want to navigate after a login you could simply navigate from the login button callback instead of using an effect.

Caching navigation object is purely a perf optimization and shouldn't impact behaviour of the app. The dependency array should also be treated as a perf optimization.

Not sure what you mean by that, and also not sure to agree. For me it's perfectly fine to navigate in an effect, and if you do, eslint will ask you to put navigate as a dependency so it should rather be stable (it would probably work fine if you just omitted it but people will complain due to eslint)

Are we talking about "navigation" or "navigate" here? Because I think

• navigation.navigate/goBack etc should be stable (usable in effects and dependencies)
• navigation object does not need to be stable (can be cached if faster impl detail, but the user shouldn't expect it to be stable in his code)

[satya164]

If you want to navigate after a login you could simply navigate from the login button callback instead of using an effect.

It depends on the architecture. This pattern will be more about frameworks like Redux where the fetching happens in a middleware and the component just receives latest object. There's no callback.

Not sure what's the intent of your code either

It's simply saying if the user was undefined before and now available, then navigate. Which usually means the user signed in. I have written code like that with redux in componentDidUpdate.

Not sure what you mean by that, and also not sure to agree.
For me it's perfectly fine to navigate in an effect

Yes it's perfectly fine to navigate in useEffect as I wrote in the code. What's wrong in that code the condition. What condition should trigger the navigation? Triggering navigation simply when the user object is truthy isn't a correct condition. It'd be weird to want to navigate every time the user object changes.

The fact that navigation object changes here is not actually very relevant. Even if we make it more stable, the navigation object will still change, just less often. It's a prop, no one should write a component assuming a prop will never change. If a React component is written like this, it could work in very specific cases, but usually it's wrong (imagine adding an event listener in componentDidMount and never updating it when props changed in componentDidUpdate).

IMO the advantage of dependency array is that it's harder to write code assuming a prop never changes. Now, how often you need to run the effect shouldn't usually impact actual behaviour. If you're making a network request, you definitely want to minimize how many times you want to run that, but if doing multiple requests breaks the behaviour of the app, there's a bug. Reducing the network requests is an optimization here.

In 5.x, the navigation object will be much more stable. It will only change when when parent navigation changes, or the index of that route changes (as opposed to current react navigation changing it when route changes). But it'll still change. So people shouldn't write code assuming that it'll never change.

The reason we need to fix the caching is because it prevents pure component optimization. But I'd strongly recommend not to write code which assumes that the props never change.

[slorber]

Yes it's perfectly fine to navigate in useEffect as I wrote in the code. What's wrong in that code the condition. What condition should trigger the navigation? Triggering navigation simply when the user object is truthy isn't a correct condition. It'd be weird to want to navigate every time the user object changes.

Ahhh yeah missed that, we don't want to navigate to authenticated screens everytime the user updates his username :) I'd rather use something like !!user / boolean for that instead.

The reason we need to fix the caching is because it prevents pure component optimization. But I'd strongly recommend not to write code which assumes that the props never change.

Yeah I see, we'd like to avoid re-rendering screens uselessly on navigation change.

But what about navigation actions (navigate/goBack). Currently, those functions are not stable, but I think it would be convenient for the users if we guaranteed stability.

People could write:

  useEffect(() => {
      if (!!user) {
          navigate("AuthenticatedNavigator")
      }
  }, [!!user])

And it would normally work fine.

But in practice, they'll have ESLint and just to comply to ESLint they'll put the navigate as dependency:

  useEffect(() => {
      if (!!user) {
          navigate("AuthenticatedNavigator")
      }
  }, [!!user, navigate])

And then we have a bug, that was actually caused by following ESLint recos.

Users can still solve it in userland and stabilize the navigate function themselves, or add an eslint-ignore, but I think it would be more convenient to just ensure they can add navigate as a dependency and expect it to be stable.

[satya164]

We could probably make them stable, depends on how the caching logic is written and we definitely should. But I still think the effect logic is wrong. User shouldn't rely on a prop being stable.

I'm thinking this way, does the effect need to run if the user object is available or does the effect need to run the first time the user object is available? If it's the later then the effect should have a condition to prevent that.

Imagine if it wasn't navigate, or the user refactors it to something else in future, passing a callback from parent instead which does some stuff + navigate. Now, because the effect is missing correct condition, the bug will happen again, this time not because navigate changed, but because that prop changed.

[slorber]

For me it looks fine to navigate every time the user becomes available. If user logs in, logs out, and logs in again, the effect/navigate should execute again (actually most people will use SwitchNavigator which will unmount/remount the unauthenticated screens after login, but they may keep them mounted and if they do, that logic makes sense to me, otherwise they won't navigate after the 2nd login)

---

We actually don't want this effect to kick in if the navigation.navigate fn changes, and personally I'd write this without the "navigate" in deps, assuming react-navigation does not guarantee stability (which is quite an implicit contract):

  useEffect(() => {
      if (isAuthenticated) {
          navigate("AuthenticatedNavigator")
      } else {
          navigate("UnauthenticatedNavigator")
      }
  }, [isAuthenticated]) 

I'm not using the hooks rules yet because I'm still on tslint in some projects, but I see how this rule of exhaustive deps can sometimes be annoying, and maybe even create bugs.

Would you recommend the user to not follow the exhaustive deps rule like I do and not put the navigate into deps?

Or do you think we should always follow the ESLint rule and handle inside the effect the potential annoying side effects that this imply? Because personally I think it sure works but also complicates a bit the code, I'd rather not follow the ESLint rule and have simpler code.

But in practice, not following the ESLint rule would mean we assume the navigate() function identity changes over time, but it's behavior is stable. ie we have navigate1 and navigate2 with 2 distinct identity, and we can call navigate1("X") or navigate2("X") and it would lead to the same result. For that we must be sure that the navigate closure does not capture any state that would lead to different results

[slorber]

Also it's possible to use a "navigateRef" which can be stable and ensure to always return latest navigation object:

const { navigationRef } = useNavigation();

  useEffect(() => {
      if (isAuthenticated) {
          navigationRef.current.navigate("AuthenticatedNavigator")
      } else {
          navigationRef.current.navigate("UnauthenticatedNavigator")
      }
  }, [isAuthenticated,navigationRef]) 

This feels a bit weird to use but would actually be safe and eslint would be ok with that.

[satya164]

For me it looks fine to navigate every time the user becomes available

Yea, but the logic also has navigate in dep array, so the condition is not only about user becoming available.

I'm just saying that the current code is incorrect, not if it'll be correct if we gurrantee that these methods are stable.

It also helps to stop thinking in terms of navigate and thinking in terms of a callback passed from parent. With such an assumption, refactoring parent may easily break child component. Adding another dep (say along with navigate, you want to pass some param from local state) will also break this code.

I see how this rule of exhaustive deps can sometimes be annoying, and maybe even create bugs.

I agree that it can be annoying. But I disagree that it'll create bugs. The rule can cause de-opts, but I can't think of a scenario where it actually creates bugs if the code is correct. Rather, it surfaces existing bugs in the code. Happy to be proven wrong.

Would you recommend the user to not follow the exhaustive deps rule like I do and not put the navigate into deps?

I'd not. Because we don't guarantee that this is stable.

So should we guarantee that all methods in navigation object will be stable as part of our API contract? I don't know. With classes it's easier to guarantee. With function components it becomes increasingly more difficult to write such code. It's also too easy to forget and break.

I'm of the opinion is how a prop is created and stored is parent's implementation detail. Child shouldn't rely on it.

Or do you think we should always follow the ESLint rule and handle inside the effect the potential annoying side effects that this imply? Because personally I think it sure works but also complicates a bit the code, I'd rather not follow the ESLint rule and have simpler code.

It does complicate the code. But it's your choice and your specific use case whether simpler code is better than correct code. I don't have any general opinion on that.

[satya164]

Maybe there should be useNavigationRef hook?

[slorber]

I have no idea what to do :)

Having useNavigationRef would work just fine, but feels a bit awkward. We could simply document a recipe maybe and see from there if people are complaining?

[slorber]

Hi @satya164

I've been thinking a bit about that and I think:

• core does not need to stabilize the functions
• the hooks package can/should stabilize and offer that guarantee as contract

It should be easier to do the memoization directly in a hook with useRef, and the users most likely to useEffect(fn,[navigate]) are also most likely to obtain the navigate function through a hook, so I think it's fine if only the hooks package offer the memoization.

=> react-navigation/hooks#40

If you think I should not support that contract please tell me.

Also feel free to reopen this issue if you think there's something to change in core (you mentioned that maybe getChildrenNavigationCache.js was not working correctly)