Back button `pop` logic race condition about expo-next-monorepo-example HOT 13 CLOSED

I'm also finding that adding an artists route helps. Currently, I only have '' and artists/:slug. Doing this helps:

 const linking = useLinkingConfig({
    prefixes: [Linking.makeUrl('/')], 
    config: {
      screens: { 
        artistsTab: {
          path: '',
          initialRouteName: 'artists',
          screens: {
            artist: 'artists/:slug',
            artists: 'artists'
          },
        },
      },
    },
  })

This is the best solution I have so far. As long as the home page is a "list" screen that can also map to another URL, it's fine. My recommendation for generalized use would be to also have a route at home that catches the same screen. That way, if you go "back" it'll go to /home, and you'll have the same screen there as /. It requires making an extra screen for Next.js, but not that bad.

from expo-next-monorepo-example.

Okay, I have a better diagnosis of the problem:

When you go back from a given screen, it will always turn the URL into the name of that screen. It seems that this pop logic is just completely off.

If you're at /artists/:id with screen name artist, and go back, it will always go back to /artist.

If you're at /venues/:id with screen name venue, and go back, it will go back to /venue. This makes no sense for behavior, of course; it should go to the previous screen in the stack.

from expo-next-monorepo-example.

I just learned one added, interesting thing:

If you want one screen to be able to go back to another, they have to both be in the same stack. So make sure that the stack you make in /pages includes all the screens that should be possible to go back to inside of it.

from expo-next-monorepo-example.

Also, looks like this logic works better for going back. Seems like getPathFromState is returning the incorrect path sometimes...so I'm checking the next state screen's path.

  const back = () => {
    if (nextRouter) {
      const nextState = stack.getStateForAction(
        navigation.getState(),
        StackActions.pop(),
        // @ts-expect-error pop and goBack don't need the dict here, it's okay
        // goBack: https://github.com/react-navigation/react-navigation/blob/main/packages/routers/src/CommonActions.tsx#L49
        // pop: https://github.com/react-navigation/react-navigation/blob/main/packages/routers/src/StackRouter.tsx#L317
        {}
      )
      if (nextState) {
        let path =
          nextState.index != undefined
            ? nextState.routes[nextState.index]?.path
            : undefined

        if (!path) {
          const getPath = linking?.options?.getPathFromState || getPathFromState
          path = getPath(nextState, linking?.options?.config)
        }

        if (path != undefined) {
          return nextRouter.replace(path)
        }
      }
    }

    navigation.goBack()
}

from expo-next-monorepo-example.

The structure for this library is like so: every single Next.js page is a React Navigation Stack.

Yeah that's what I do for my apps. We should really communicate about this and write a complete guide on the architecture.

Just tested using dynamic imports to improve tree shaking and it's working well! Good idea, thanks!

from expo-next-monorepo-example.

I think the right solution will be to somehow use the first-class citizen getPathFromState inside of the back button: https://reactnavigation.org/docs/navigation-container#linkinggetpathfromstate

We just have to figure out how to get the state after popping that screen off. Is it as simple as making the index of the state one lower? The difficult part is recursively getting down to the current screen and knowing which one has to pop. After all, that is what the .pop() function is supposed to do for us...

from expo-next-monorepo-example.

Another solution is to not use the '' URL in a stack haha. Not ideal.

from expo-next-monorepo-example.

I'm also finding that adding an artists route helps. Currently, I only have '' and artists/:slug. Doing this helps:

 const linking = useLinkingConfig({
    prefixes: [Linking.makeUrl('/')], 
    config: {
      screens: { 
        artistsTab: {
          path: '',
          initialRouteName: 'artists',
          screens: {
            artist: 'artists/:slug',
            artists: 'artists'
          },
        },
      },
    },
  })

from expo-next-monorepo-example.

I made a redirect in my next config:

/**
 * @type {import('next').NextConfig}
 */
const nextConfig = {
  experimental: {
    esmExternals: 'loose',
  },
  images: {
    disableStaticImages: true,
  },
  async redirects() {
    return [
      {
        source: '/',
        destination: '/artists',
        permanent: false,
      },
    ]
  },
}

Still looking into better solutions, since we of course want to be able to use the root endpoint.

from expo-next-monorepo-example.

This makes no sense for behavior, of course; it should go to the previous screen in the stack.

Damn, this is weird! Can you share your current implementation for router.pop() please? Just want to make sure we have the same before digging into this. I don't think I have the same behaviour, a repro would be helpful.

from expo-next-monorepo-example.

Yeah! here's my back button:

import {
  HeaderBackButton as ReactNavigationHeaderBackButton,
  HeaderBackButtonProps,
} from '@react-navigation/elements'
import { useRouter } from '../use-router'

import { useRouter as useNextRouter } from 'next/router'
import { StackRouter } from '@react-navigation/routers'
import {
  StackActions,
  getPathFromState,
  CommonActions,
} from '@react-navigation/native'
import { NativeStackScreenProps } from '@react-navigation/native-stack'

// this is scary...
// @ts-expect-error but react navigation doesn't expose LinkingContext 😬
import LinkingContext from '@react-navigation/native/lib/module/LinkingContext'
import { LinkingOptions } from '@react-navigation/native'

import { useContext } from 'react'

// hack to access getStateForAction from react-navigation's stack
//
const stack = StackRouter({})

export function HeaderBackButton({
  navigation,
  ...props
}: HeaderBackButtonProps & {
  navigation: NativeStackScreenProps<any>['navigation']
}) {
  const linking = useContext(LinkingContext) as
    | {
        options?: LinkingOptions<ReactNavigation.RootParamList>
      }
    | undefined
  const nextRouter = useNextRouter()
 
  if (!props.canGoBack) {
    return null
  }
  const back = () => {
    if (nextRouter) {
      const nextState = stack.getStateForAction(
        navigation.getState(),
        StackActions.pop(),
        // @ts-expect-error pop and goBack don't need the dict here, it's okay
        // goBack: https://github.com/react-navigation/react-navigation/blob/main/packages/routers/src/CommonActions.tsx#L49
        // pop: https://github.com/react-navigation/react-navigation/blob/main/packages/routers/src/StackRouter.tsx#L317
        {}
      )
      console.log('[back]', {
        nextState,
        state: navigation.getState(),
      })
      if (nextState) {
        const getPath = linking?.options?.getPathFromState || getPathFromState
        const path = getPath(nextState, linking?.options?.config)

        if (path != undefined) {
          return nextRouter.replace(path)
        }
      }
    }

    navigation.goBack()
  }

  return <ReactNavigationHeaderBackButton {...props} onPress={back} />
}

These are my navigation options:

export const useNativeStackNavigationOptions = (): React.ComponentProps<
  typeof NativeStack['Navigator']
>['screenOptions'] => {
  const sx = useSx()
  const { theme } = useDripsyTheme()

  const isDrawer = useIsDrawer() 

  return useMemo(
    () =>
      ({
        navigation,
      }: {
        navigation: NativeStackScreenProps<NativeStackParams>['navigation']
      }) => ({
        headerTintColor: theme.colors.text,
        headerTitleStyle: {
          fontFamily: theme.customFonts[theme.fonts.root][500],
        },
        headerShadowVisible: false,
        contentStyle: {
          flex: 1,
        }, 
        headerLeft: Platform.select({
          web(props) {
            return <HeaderBackButton {...props} navigation={navigation} />
          },
        }),
        cardStyle: {
          flex: 1,
          backgroundColor: 'transparent',
        }, 
      }),
    [ 
      sx,
      theme.colors.border,
      theme.colors.text,
      theme.customFonts,
      theme.fonts.root,
    ]
  )
}

from expo-next-monorepo-example.

Omg, that actually solved all of this. lol. wow. I should have thought this through more.

I'll need to document this really clearly.

Walkthrough

Imagine you have these routes

/artists
/artists/drake
/artists/albums/songs/take-care

In this case, if you open /artists/drake, then going back should always go back to /artists.

The structure for this library is like so: every single Next.js page is a React Navigation Stack.

So let's look at why this was breaking for me:

What I did incorrectly

For each page, I had a stack like this:

pages/artists/[slug].tsx

export function ArtistPage() {
  const screenOptions = useNativeStackNavigationOptions()
  return (
    <NativeStack.Navigator screenOptions={screenOptions}>
      <NativeStack.Screen
        name="artist"
        component={ArtistScreen}
        getId={({ params }) => params?.slug}
      />
    </NativeStack.Navigator>
  )
}

This seemed harmless. But when I click goBack, React Navigation is looking in this stack, and thinking, okay, I'll just send you back from here, which would just be the artist screen, without any params. So it goes to /artist.

In retrospect, this was a pretty dumb mistake from me.

The solution

For screens that should stack on top of others, it's important that their stack contains every screen involved. For example, if you want to open /artists/djkhaled, and have it be able to go back to /artists, it's important for the stack to have the artists screen. That way, React Navigation says, okay, that is the previous screen in this stack.

Recall that we defined artists as initialRouteName in linking config:

const linking = useLinkingConfig({
  prefixes: [Linking.makeUrl('/')],
  config: {
    screens: {
      artistsTab: {
        path: '',
        initialRouteName: 'artists',
        screens: {
          artist: 'artists/:slug',
          artists: 'artists'
        }
      }
    }
  }
})

In order for this to work, you need to render artists in the initial stack, even if you're on the artists/:slug screen.

So here is what our pages/artists/[slug].tsx actually looks like:

// pages/artists/[slug]

export default function ArtistPage() {
  const screenOptions = useNativeStackNavigationOptions()
  return (
    <NativeStack.Navigator screenOptions={screenOptions}>
      <NativeStack.Screen
        name="artist"
        component={ArtistScreen}
        getId={({ params }) => params?.slug}
      />
      <NativeStack.Screen
        name="artists" // this is added
        component={ArtistsScreen}
      />
    </NativeStack.Navigator>
  )
}

Since this stack has both the /artists/[slug] and /artists screen, we can use the same stack for /pages/artists:

// pages/artist

export { default } from './[slug]

Thinking out loud

The remaining things I write are unconfirmed thoughts that I still need to test. I'm writing them so that I can try them out and see what works. The above is the right solution. The below might be wrong.

We now have a working /artists and /artists/[slug] screen.

But why should the /artists route load in the code for /artists/[slug]? This feels unnecessary, right?

After all, /artists never has to go back to any screen; it is the root of its stack.

What if /pages/artists only exported its own screen:

// pages/artists

export function ArtistPage() {
  const screenOptions = useNativeStackNavigationOptions()
  return (
    <NativeStack.Navigator screenOptions={screenOptions}>
      <NativeStack.Screen name="artists" component={ArtistsScreen} />
    </NativeStack.Navigator>
  )
}

Since artists is the root screen of this stack, and will never go back anywhere, it's fine to only include artists in this stack.

One concern I have with this approach is, we will lose the scroll position of /artists when we open an artist, since it will unmount.

So a better solution is probably to still include the artist screen inside of /artists, loaded dynamically:

// pages/artists

const ArtistScreen = dynamic(() => import('screens/artist'))

export function ArtistPage() {
  const screenOptions = useNativeStackNavigationOptions()
  return (
    <NativeStack.Navigator screenOptions={screenOptions}>
      <NativeStack.Screen name="artist" component={ArtistScreen}
        getId={({ params }) => params?.slug} />
      <NativeStack.Screen name="artists" component={ArtistsScreen} />
    </NativeStack.Navigator>
  )
}

Retaining scroll position for shallow routes

The above will technically still lose scroll position, because Next.js will fully unmount the /artists route in favor of /artists/[slug].

The solution to get around that would be to navigate to /artists?slug=djkhaled with shallow routing, and then tell React Navigation to treat that URL as /artists/:slug.

const { push } = useRouter()

const openArtists = () =>
  push('/artists?slug=djkhaled', '/artists/djhkaled', { shallow: true })

Here we're using the as value in push to set the URL in the address bar to /artists/djkhaled.

The thing is, React Navigation uses the pathname (the first argument), not the asPath, to trigger navigations.

So I see two possible solutions:

1. Create a "redirect" in linking, using getPathFromState (meh)

const linking = useLinkingConfig({
  prefixes: [Linking.makeUrl('/')],
  getPathFromState(state, options) {
   const path = getPathFromState(state, options)

   if (path.startsWith('/artists?slug=') {
     return `/artists/${path.split('/artists?slug=')[1]}`
   }

  return path
  },
  config: {
    screens: {
      artistsTab: {
        path: '',
        initialRouteName: 'artists',
        screens: {
          artist: 'artists/:slug',
          artists: 'artists'
        }
      }
    }
  }
})

Provide a flag inside of push for native

const { push } = useRouter()

const openArtists = () =>
  push('/artists?slug=djkhaled', '/artists/djhkaled', {
    shallow: true,
    native: {
      useAsPath: true
    }
  })

Tree shaking /artists/[slug]

In the /artists page, we dynamically imported artist, the screen that goes to /artists/[slug].

In the /artists/[slug] page, we can do the same: dynamically import artists, the screen for /artists:

// artists/[slug]

const ArtistsScreen = dymamic(() => import('screens/artists'))

export function ArtistPage() {
  const screenOptions = useNativeStackNavigationOptions()
  return (
    <NativeStack.Navigator screenOptions={screenOptions}>
      <NativeStack.Screen
        name="artist"
        component={ArtistScreen}
        getId={({ params }) => params?.slug}
      />
      <NativeStack.Screen
        name="artists"
        component={ArtistsScreen}
      />
    </NativeStack.Navigator>
  )
}

I'll be testing these things out.

Notice that we use the same screens everywhere, but the way they get imported/displayed in a given page differs. I'll keep testing these things, but overall, the race condition is solved by this comment.

Let me know if anything is unclear.

from expo-next-monorepo-example.

sweet. will def have this in the dedicated docs

from expo-next-monorepo-example.