Skip to content
This repository was archived by the owner on Dec 3, 2022. It is now read-only.

Add stable actions on useNavigation #54

Closed
wants to merge 3 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 138 additions & 2 deletions src/Hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,154 @@ import {
EventType,
} from 'react-navigation';

export function useNavigation<S>(): NavigationScreenProp<S & NavigationRoute> {
function useNavigationSafe<S>(): NavigationScreenProp<S & NavigationRoute> {
const navigation = useContext(NavigationContext) as any; // TODO typing?

if (!navigation) {
throw new Error(
"react-navigation hooks require a navigation context but it couldn't be found. " +
"Make sure you didn't forget to create and render the react-navigation app container. " +
'If you need to access an optional navigation object, you can useContext(NavigationContext), which may return'
);
}

return navigation;
}

function useStableCallback(cb: (...args: any) => any) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const ref = useRef(cb);

useLayoutEffect(() => {
ref.current = cb;
}, [cb]);
Comment on lines +37 to +39
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@slorber slorber Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)


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


type NavigationCommonActions<S> = Pick<
NavigationScreenProp<S & NavigationRoute>,
| 'navigate'
| 'goBack'
| 'addListener'
| 'isFocused'
| 'setParams'
| 'getParam'
| 'dispatch'
| 'dangerouslyGetParent'
| 'isFirstRouteInParent'
>;

type NavigationDrawerActions<S> = Pick<
NavigationScreenProp<S & NavigationRoute>,
'openDrawer' | 'closeDrawer' | 'toggleDrawer'
>;

type NavigationStackActions<S> = Pick<
NavigationScreenProp<S & NavigationRoute>,
'push' | 'pop' | 'popToTop' | 'replace' | 'reset' | 'dismiss'
>;

type NavigationActions<S> = NavigationCommonActions<S> &
Partial<NavigationDrawerActions<S>> &
Partial<NavigationStackActions<S>>;

type Navigation<S> = NavigationActions<S> &
Omit<NavigationScreenProp<S & NavigationRoute>, keyof NavigationActions<S>>;

/*
This is intended to solve: https://github.com/react-navigation/hooks/issues/40
Read more here: https://github.com/facebook/react/issues/16956
*/
function useStableActions<S>(navigation: NavigationScreenProp<S & NavigationRoute>): NavigationActions<S & NavigationRoute> {
// common actions
const navigate = useStableCallback(navigation.navigate);
const goBack = useStableCallback(navigation.goBack);
const addListener = useStableCallback(navigation.addListener);
const isFocused = useStableCallback(navigation.isFocused);
const setParams = useStableCallback(navigation.setParams);
const getParam = useStableCallback(navigation.getParam);
const dispatch = useStableCallback(navigation.dispatch);
const dangerouslyGetParent = useStableCallback(navigation.dangerouslyGetParent);
const isFirstRouteInParent = useStableCallback(navigation.isFirstRouteInParent);

// drawer navigator, actions
const openDrawer = useStableCallback(navigation.openDrawer);
const closeDrawer = useStableCallback(navigation.closeDrawer);
const toggleDrawer = useStableCallback(navigation.toggleDrawer);

// stack navigator actions
const push = useStableCallback(navigation.push);
const pop = useStableCallback(navigation.pop);
const popToTop = useStableCallback(navigation.popToTop);
const replace = useStableCallback(navigation.replace);
const reset = useStableCallback(navigation.reset);
const dismiss = useStableCallback(navigation.dismiss);

let result: NavigationActions<S & NavigationRoute> = {
// common actions
navigate,
goBack,
addListener,
isFocused,
setParams,
getParam,
dispatch,
dangerouslyGetParent,
isFirstRouteInParent,

// drawer navigator actions
openDrawer,
closeDrawer,
toggleDrawer,

// stack navigator actions
push,
pop,
popToTop,
replace,
reset,
dismiss,
};

if (navigation.openDrawer) {
result = {
...result,

// drawer navigator actions
openDrawer,
closeDrawer,
toggleDrawer,
}
}

if (navigation.push) {
result = {
...result,

// stack navigator actions
push,
pop,
popToTop,
replace,
reset,
dismiss,
}
}

return result;
}

export function useNavigation<S>(): Navigation<S> {
const navigation = useNavigationSafe<S>()
const actions = useStableActions<S>(navigation);

return {
...navigation,
...actions,
};
}

export function useNavigationParam<T extends keyof NavigationParams>(
paramName: T
) {
Expand All @@ -44,7 +180,7 @@ export function useNavigationKey() {
}

// Useful to access the latest user-provided value
const useGetter = <S>(value: S): (() => S) => {
function useGetter<S>(value: S): (() => S) {
const ref = useRef(value);
useLayoutEffect(() => {
ref.current = value;
Expand Down