From 8a32476de5925997d33d54d9e1003dc2cfb2435e Mon Sep 17 00:00:00 2001 From: evanbacon Date: Thu, 29 Jun 2023 15:47:52 -0700 Subject: [PATCH 1/3] fix focus renders --- .../src/HMRClient.native.ts | 4 +- .../src/__tests__/useFocusEffect.test.tsx | 73 +++++++++++++++++++ .../src/global-state/router-store.tsx | 19 +++++ .../src/link/useLoadedNavigation.ts | 60 --------------- packages/expo-router/src/useFocusEffect.tsx | 24 ++++-- 5 files changed, 112 insertions(+), 68 deletions(-) create mode 100644 packages/expo-router/src/__tests__/useFocusEffect.test.tsx delete mode 100644 packages/expo-router/src/link/useLoadedNavigation.ts diff --git a/packages/expo-metro-runtime/src/HMRClient.native.ts b/packages/expo-metro-runtime/src/HMRClient.native.ts index 82446bf77..a74261da0 100644 --- a/packages/expo-metro-runtime/src/HMRClient.native.ts +++ b/packages/expo-metro-runtime/src/HMRClient.native.ts @@ -1 +1,3 @@ -module.exports = require("react-native/Libraries/Utilities/HMRClient"); +const HMRClient = require("react-native/Libraries/Utilities/HMRClient"); + +export default HMRClient; diff --git a/packages/expo-router/src/__tests__/useFocusEffect.test.tsx b/packages/expo-router/src/__tests__/useFocusEffect.test.tsx new file mode 100644 index 000000000..f6a2afbe7 --- /dev/null +++ b/packages/expo-router/src/__tests__/useFocusEffect.test.tsx @@ -0,0 +1,73 @@ +import React, { Text } from "react-native"; + +import { router, useFocusEffect } from "../exports"; +import { Stack } from "../layouts/Stack"; +import { act, renderRouter, screen } from "../testing-library"; + +it("does not re-render extraneously", () => { + const events: string[] = []; + renderRouter({ + _layout: () => { + useFocusEffect(() => { + events.push("_layout"); + }); + + return ; + }, + index: () => { + // eslint-disable-next-line react-hooks/rules-of-hooks + useFocusEffect(() => { + events.push("index"); + }); + return index; + }, + two: () => { + // eslint-disable-next-line react-hooks/rules-of-hooks + useFocusEffect(() => { + events.push("two"); + }); + return two; + }, + }); + + expect(screen).toHavePathname("/"); + expect(events).toEqual(["_layout", "index"]); + + act(() => router.push("/two")); + expect(screen).toHavePathname("/two"); + expect(events).toEqual(["_layout", "index", "two"]); + act(() => router.push("/")); + expect(screen).toHavePathname("/"); + expect(events).toEqual(["_layout", "index", "two", "index"]); +}); + +it("can navigate instantly after mounting", () => { + const events: string[] = []; + renderRouter({ + _layout: () => { + useFocusEffect(() => { + events.push("_layout"); + router.push("/two"); + }); + + return ; + }, + index: () => { + // eslint-disable-next-line react-hooks/rules-of-hooks + useFocusEffect(() => { + events.push("index"); + }); + return index; + }, + two: () => { + // eslint-disable-next-line react-hooks/rules-of-hooks + useFocusEffect(() => { + events.push("two"); + }); + return two; + }, + }); + + expect(screen).toHavePathname("/two"); + expect(events).toEqual(["_layout", "two"]); +}); diff --git a/packages/expo-router/src/global-state/router-store.tsx b/packages/expo-router/src/global-state/router-store.tsx index 4bc274eb3..543d19b39 100644 --- a/packages/expo-router/src/global-state/router-store.tsx +++ b/packages/expo-router/src/global-state/router-store.tsx @@ -38,6 +38,7 @@ export class RouterStore { rootStateSubscribers = new Set<() => void>(); storeSubscribers = new Set<() => void>(); + private readySubscribers = new Set<() => void>(); linkTo = linkTo.bind(this); getSortedRoutes = getSortedRoutes.bind(this); @@ -182,7 +183,25 @@ export class RouterStore { requestAnimationFrame(() => _internal_maybeHideAsync()); } this.isReady = true; + + for (const subscriber of this.readySubscribers) { + subscriber(); + } }; + + /** Called once when the navigation is ready. Specifically added for `useFocusEvent` + redirection. */ + onceReady = (callback: () => void) => { + if (this.navigationRef.isReady()) { + callback(); + } else { + const remove = () => { + callback(); + this.readySubscribers.delete(remove); + }; + this.readySubscribers.add(remove); + } + }; + subscribeToRootState = (subscriber: () => void) => { this.rootStateSubscribers.add(subscriber); return () => this.rootStateSubscribers.delete(subscriber); diff --git a/packages/expo-router/src/link/useLoadedNavigation.ts b/packages/expo-router/src/link/useLoadedNavigation.ts deleted file mode 100644 index 1121c3b15..000000000 --- a/packages/expo-router/src/link/useLoadedNavigation.ts +++ /dev/null @@ -1,60 +0,0 @@ -import { NavigationProp, useNavigation } from "@react-navigation/native"; -import { useCallback, useState, useEffect, useRef } from "react"; - -import { useExpoRouter } from "../global-state/router-store"; - -type GenericNavigation = NavigationProp; - -/** Returns a callback which is invoked when the navigation state has loaded. */ -export function useLoadedNavigation() { - const { navigationRef } = useExpoRouter(); - const navigation = useNavigation(); - const isMounted = useRef(true); - const pending = useRef<((navigation: GenericNavigation) => void)[]>([]); - - useEffect(() => { - isMounted.current = true; - return () => { - isMounted.current = false; - }; - }, []); - - const flush = useCallback(() => { - if (isMounted.current) { - const pendingCallbacks = pending.current; - pending.current = []; - pendingCallbacks.forEach((callback) => { - callback(navigation); - }); - } - }, [navigation]); - - useEffect(() => { - if (navigationRef.current) { - flush(); - } - }, [flush]); - - const push = useCallback( - (fn: (navigation: GenericNavigation) => void) => { - pending.current.push(fn); - if (navigationRef.current) { - flush(); - } - }, - [flush] - ); - - return push; -} - -export function useOptionalNavigation(): GenericNavigation | null { - const [navigation, setNavigation] = useState(null); - const loadNavigation = useLoadedNavigation(); - - useEffect(() => { - loadNavigation((nav) => setNavigation(nav)); - }, []); - - return navigation; -} diff --git a/packages/expo-router/src/useFocusEffect.tsx b/packages/expo-router/src/useFocusEffect.tsx index 21dfe2ca7..a73dab965 100644 --- a/packages/expo-router/src/useFocusEffect.tsx +++ b/packages/expo-router/src/useFocusEffect.tsx @@ -1,8 +1,9 @@ // A fork of `useFocusEffect` that waits for the navigation state to load before // running the effect. This is especially useful for native redirects. +import { useNavigation } from "@react-navigation/native"; import * as React from "react"; -import { useOptionalNavigation } from "./link/useLoadedNavigation"; +import { useExpoRouter } from "./global-state/router-store"; type EffectCallback = () => undefined | void | (() => void); @@ -17,8 +18,8 @@ export function useFocusEffect( effect: EffectCallback, do_not_pass_a_second_prop?: never ) { - const navigation = useOptionalNavigation(); - + const { navigationRef, onceReady } = useExpoRouter(); + const navigation = useNavigation(); if (do_not_pass_a_second_prop !== undefined) { const message = "You passed a second argument to 'useFocusEffect', but it only accepts one argument. " + @@ -79,8 +80,8 @@ export function useFocusEffect( } }; - // We need to run the effect on intial render/dep changes if the screen is focused - if (navigation.isFocused()) { + // We need to run the effect on initial render/dep changes if the screen is focused + if (navigation.isFocused() && navigationRef.isReady()) { cleanup = callback(); isFocused = true; } @@ -96,8 +97,17 @@ export function useFocusEffect( cleanup(); } - cleanup = callback(); - isFocused = true; + if (!navigationRef.isReady()) { + onceReady(() => { + if (!isFocused && navigation.isFocused()) { + cleanup = callback(); + isFocused = true; + } + }); + } else { + cleanup = callback(); + isFocused = true; + } }); const unsubscribeBlur = navigation.addListener("blur", () => { From 04d9992d49597babe8440b02a3af2b069a391c5c Mon Sep 17 00:00:00 2001 From: evanbacon Date: Thu, 29 Jun 2023 16:30:31 -0700 Subject: [PATCH 2/3] Update Link.tsx --- packages/expo-router/src/link/Link.tsx | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/expo-router/src/link/Link.tsx b/packages/expo-router/src/link/Link.tsx index 0391ba684..33fa5146f 100644 --- a/packages/expo-router/src/link/Link.tsx +++ b/packages/expo-router/src/link/Link.tsx @@ -5,7 +5,7 @@ import { Slot } from "@radix-ui/react-slot"; import * as React from "react"; import { GestureResponderEvent, Platform } from "react-native"; -import { useRouter } from "../hooks"; +import { router } from "../imperative-api"; import { useFocusEffect } from "../useFocusEffect"; import { Href, resolveHref } from "./href"; import useLinkToPathProps from "./useLinkToPathProps"; @@ -28,13 +28,15 @@ export interface LinkProps extends Omit { /** Redirects to the href as soon as the component is mounted. */ export function Redirect({ href }: { href: Href }) { - const router = useRouter(); useFocusEffect(() => { - try { - router.replace(href); - } catch (error) { - console.error(error); - } + // TODO: Remove this + setTimeout(() => { + try { + router.replace(href); + } catch (error) { + console.error(error); + } + }); }); return null; } From efc49a270bcd61c95e0ac6a6ebf3f735f5084306 Mon Sep 17 00:00:00 2001 From: evanbacon Date: Thu, 29 Jun 2023 17:22:12 -0700 Subject: [PATCH 3/3] wip -- test is broken --- packages/expo-router/src/__tests__/smoke.test.tsx | 13 ++++++++----- packages/expo-router/src/link/Link.tsx | 13 +++++-------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/expo-router/src/__tests__/smoke.test.tsx b/packages/expo-router/src/__tests__/smoke.test.tsx index 2e7e80982..10233b74e 100644 --- a/packages/expo-router/src/__tests__/smoke.test.tsx +++ b/packages/expo-router/src/__tests__/smoke.test.tsx @@ -109,17 +109,20 @@ it("nested layouts", async () => { "(app)/_layout": AppLayout, "(app)/index": Index, "(app)/(tabs)/_layout": TabsLayout, - "(app)/(tabs)/home/_layout": StackLayout, + "(app)/(tabs)/home/_layout": { + unstable_settings: { initialRouteName: "index" }, + default: StackLayout, + }, "(app)/(tabs)/home/index": Home, "(app)/(tabs)/home/nested": HomeNested, }); expect(await screen.findByText("HomeNested")).toBeOnTheScreen(); - expect(AppLayout).toHaveBeenCalledTimes(3); - expect(TabsLayout).toHaveBeenCalledTimes(2); - expect(StackLayout).toHaveBeenCalledTimes(2); + expect(AppLayout).toHaveBeenCalledTimes(4); + expect(TabsLayout).toHaveBeenCalledTimes(3); + expect(StackLayout).toHaveBeenCalledTimes(3); expect(Index).toHaveBeenCalledTimes(1); - expect(Home).toHaveBeenCalledTimes(1); + expect(Home).toHaveBeenCalledTimes(2); expect(HomeNested).toHaveBeenCalledTimes(1); }); diff --git a/packages/expo-router/src/link/Link.tsx b/packages/expo-router/src/link/Link.tsx index 33fa5146f..ed283dfbb 100644 --- a/packages/expo-router/src/link/Link.tsx +++ b/packages/expo-router/src/link/Link.tsx @@ -29,14 +29,11 @@ export interface LinkProps extends Omit { /** Redirects to the href as soon as the component is mounted. */ export function Redirect({ href }: { href: Href }) { useFocusEffect(() => { - // TODO: Remove this - setTimeout(() => { - try { - router.replace(href); - } catch (error) { - console.error(error); - } - }); + try { + router.replace(href); + } catch (error) { + console.error(error); + } }); return null; }