Add stable actions on useNavigation

[neiker]

This solves: #40

[FireyFly]

Hm, I might be missing something, but do we need to run this as an effect rather than just straight-up ref.current = cb;? The ref.current in the function passed to useCallback should refer to the value-at-time-of-invocation anyway, so I don't think it should lead to staleness.

[neiker]

Good point, by default I put all the side effects on the commit phase or layout phase. But maybe in this case is not necessary and it can be runned on the render phase as it's only a reasigment and don't interfers on the render.

Maybe changing the ref on render phase can be considered an unexpected side effect?
https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects

[neiker]

oh and this pattern is already used on the useGetter function below.

[slorber]

Hi,

Doing this kind of side effect looks unsafe to me with concurrent mode. React might decide to start rendering something based on predictions, to warm up the next view, and that transition may end up to never happen. Dan Abramov mentioned that recently on Twitter.

I think it's safer and good enough to use an effect for this usecase

and yes there's the "useGetter" already ;)

[slorber]

Hi,

Your solution is something I had in mind to solve this problem.

Maybe with better TS support we could handle it this way, but I'm not very convinced this is a good solution.

Unfortunately, I don't have a better solution so far :/ exposing a "useNavigationRef" could work but feels weird.

@satya164 any opinion on this PR?

[slorber]

in terms of typing this is not good enough, all wrapped actions will be any[] => any

[neiker]

The public api will have the same types as now, the (...args: any) => any will remain private. It's bad, but not so bad.

[slorber]

before, on a stack navigation, openDrawer would be undefined. Now it's a callback that will crash with "undefined is not a function", not exactly the same ;)

[neiker]

You are right! On v4 this also change since StackNavigator is in another package. I'll take a look on this.

[satya164]

I think this is not the responsibility of react-native-hooks since this changes the semantics of how the navigation object behaves. Refactoring between props.navigation and useNavigation should not change the behaviour or break code. This kind of change should be done in React Navigation core.

Also, we should not hardcode these methods on the navigation object. As you pointed out, methods like openDrawer will now exist on stack navigator which shouldn't be the case. It also means that for navigators that are not handled here will behave differently, for example, tab navigator has a jumpTo method which won't be stable like all other methods here.

A better solution is to send a PR to core to make these methods stable :)

[neiker]

There's an issue already closed on core repo for this:
https://github.com/react-navigation/core/issues/71

This should be here because its too dificult to make that change on core right now and also because v5 it's coming and this package will be deprecated.

[neiker]

Doing more research I found there's too many posible bugs with this implementation.
More info here: facebook/react#16956

Maybe it's just simpler let this as it is and add a warning so users can just add this helper to theirs code base:

function useStableCallback(cb: (...args: any) => any) {
  const ref = useRef(cb);

  useLayoutEffect(() => {
    ref.current = cb;
  }, [cb]);

  return useCallback((...args) => ref.current(...args), [ref]);
};

[satya164]

its too dificult to make that change on core right now

I don't think it's too difficult. The action creators are created in one place it should be possible to use a similar approach by storing these in a cache object instead ref.

It's more work but it's also correct and handles way more cases than you can handle here without complicating code.

and also because v5 it's coming and this package will be deprecated

That doesn't mean we should make it inconsistent with the version of React Navigation we use it with.

Anyway, as I already mentioned, there are many issues with this approach.

• Hardcoding shouldn't be done because
  • There can be multiple navigators. e.g. currently your code doesn't handle jumpTo for tab. Custom navigators can have their own methods as well.
  • Users can provide their custom action creators. This doesn't handle it.
  • There are other methods in React Navigation as well. Methods like emit are not covered and hardcoding means any changes/additions will make this code incorrect.
• It makes navigation object semantically different. You refactor from useNavigation to props.navigation and now the code breaks. This shouldn't happen.
• Methods like getParam and isFirstRouteInParent should not be stable.

[slorber]

Good points, thanks @satya164

I'm closing the PR then

It makes navigation object semantically different. You refactor from useNavigation to props.navigation and now the code breaks. This shouldn't happen.

Agree on this, and also a reason I don't think we should fix it here but in core instead.

Temporarily, I think the best option would be to recommend users to fix this in userland, with a workaround like

function useNavigationRef() {
  const navigation = useNavigation();
  const ref = useRef(navigation);

  useLayoutEffect(() => {
    ref.current = navigation;
  }, [navigation]);

  return ref;
};

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

What do you think?