Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Update useBlocker hook to provide current and next location when conditionally blocking #1790

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tomrehnstrom
Copy link

Currently, when using the useBlocker hook there is no way to ascertain where the navigation being blocked is headed.

For my usage of blocking, this is necessary to allow navigation within a selected bunch of nested routes, but not allowing the user to exit the base route. Thought this could be a useful feature to have.

@schiller-manuel
Copy link
Contributor

we wanted to follow react-router's API for this:

https://reactrouter.com/en/main/hooks/use-blocker

can you adjust your PR to match it?

also, the location should be typesafe instead of just a string.

additionally, we would also need tests for useBlocker

@tomrehnstrom
Copy link
Author

we wanted to follow react-router's API for this:

https://reactrouter.com/en/main/hooks/use-blocker

can you adjust your PR to match it?

also, the location should be typesafe instead of just a string.

additionally, we would also need tests for useBlocker

Yes sounds good, I will update the PR as soon as possible

@tomrehnstrom
Copy link
Author

tomrehnstrom commented Jun 20, 2024

This implementation should now be providing the blockerFn inuseBlocker with a similar API to the hook in react-router. This also makes the blocker catch backwards navigation using the browser back buttons, which is very beneficial for our use case.

Things left to do:

  • Tests
  • Update docs/examples?

I am not quite sure how to do with the old blockerFn since it now behaves in a completely different way now. So any hints from someone on this would be appreciated.

Would also like to give a quick pitch for a different interface that could be used for useBlocker which I have implemented in our codebase with these changes:

import React from 'react'
import type { BlockerFn } from '../../lib/router/qudak-router'
import { BlockerFn, useRouter } from '@tanstack/react-router'

type BlockerOpts = {
  blockerFn: BlockerFn
  disabled?: boolean
}

export function useBlocker({ blockerFn, disabled = false }: BlockerOpts) {
  const { history } = useRouter()

  React.useEffect(() => {
    return disabled ? undefined : history.block(blockerFn as BlockerFn)
  }, [blockerFn, disabled, history])
}

Which gives very clean implementations for custom UIs using hooks and could look something like this:

  useBlocker({
    blockerFn: () => {
      if (blockerDisabled && blockerDisabled.current) {
        blockerDisabled.current = false
        return false
      }
      if (!blockerDisabled && _blockerDisabled.current) {
        _blockerDisabled.current = false
        return false
      }
      const shouldBlock = props.blockerFn()
      if (!shouldBlock) return false

      const promise = new Promise<boolean>((resolve) => {
        modals.open({
          title: 'Are you sure you want to leave?',
          children: (
            <SaveBlocker
              onSave={onSave}
              onDiscard={onDiscard}
              close={() => {
                modals.closeAll()
                resolve(false)
              }}
              confirm={() => {
                modals.closeAll()
                resolve(true)
              }}
              reject={() => {
                modals.closeAll()
                resolve(true)
              }}
            />
          ),
          onClose: () => resolve(false)
        })
      })

      return promise
    },
    disabled
  })

Any thoughts on this?

@tomrehnstrom tomrehnstrom changed the title feat: provide navigation task to useBlocker blockerFn feat: Update useBlocker hook to provide current and next location when conditionally blocking Jun 20, 2024
@schiller-manuel
Copy link
Contributor

This would be a breaking change to useBlocker, right? then we cannot do it (now, only possible in v2).

@chorobin
Copy link
Contributor

chorobin commented Jun 20, 2024

While this might be out of scope for this PR. I think we can likely do much better from a type safety point of view for useBlocker. Correct me if I'm wrong but useBlocker blocks a navigation and we usually describe navigations with from and to.

I was imagining a more type safe hook.

useBlocker({ from: Route.fullPath, to: '..', blockerFn: ({ fromMatch, toMatch }) => // have I saved? })
  • from would describe a route matching the current location and would narrow the fromMatch passed to blockerFn for the matching route. This will be optional, fromMatch will be loose if not provided
  • to would describe a route matching the destination location and would narrow the toMatch passed to blockerFn for the matching route. This will be optional, toMatch will be loose if not provided
  • blockerFn determines if the navigation should continue or block.
  • fromMatch/toMatch is a match like from useMatch which will provide the usual params, search, context, etc

When from or to is provided the blockerFn is only called when navigating to the location described by these two properties. If they are not provided it will always call blockerFn for any navigation

@TanStack TanStack deleted a comment from BrendanC23 Jun 21, 2024
@schiller-manuel
Copy link
Contributor

we should make this as typesafe as possible, as per @chorobin's comment.
@tomrehnstrom can you tackle this? if you need support let us know

@ThomasStock
Copy link

@tomrehnstrom, are you planning to continue working on this PR?

Does anyone know a quick and dirty workaround I can apply in the meanwhile?

@tomrehnstrom
Copy link
Author

tomrehnstrom commented Aug 5, 2024

@tomrehnstrom, are you planning to continue working on this PR?

Does anyone know a quick and dirty workaround I can apply in the meanwhile?

Yes, but have been quite busy elsewhere recently. The way i have solved this quickly in our codebase is just to copy the history package index.ts file and make the changes required to make it work, and then use it as a custom history:

const history = createCustomBrowserHistory()

export const router = createRouter({
  routeTree,
  defaultPendingComponent: FullWidthLoader,
  defaultErrorComponent: RouterSuspenseErrorFallback,
  defaultNotFoundComponent: Error404,
  context: {
    auth: undefined!,
    queryClient
  },
  defaultPreload: 'intent',
  // Since we're using React Query, we don't want loader calls to ever be stale
  // This will ensure that the loader is always called when the route is preloaded or visited
  defaultPreloadStaleTime: 0,
  history
})

With the changes in my first commits to this pr

Then written a custom blocker hook like this:

type BlockerOpts = {
  blockerFn: CustomBlockerFn
  disabled?: boolean
}

export function useCustomBlocker({ blockerFn, disabled = false }: BlockerOpts) {
  const { history } = useRouter()

  React.useEffect(() => {
    return disabled ? undefined : history.block(blockerFn as BlockerFn)
  }, [blockerFn, disabled, history])
}

@ThomasStock
Copy link

Thanks @tomrehnstrom, I needed this urgently and managed to make do with the suggested temporary solution for now.

@tomrehnstrom
Copy link
Author

Hello @schiller-manuel, I have finally got this PR into a pretty nice shape. However, I am not sure about the way I implemented route from/to matching in the useBlocker hook, any suggestions there would be appreciated.

Copy link

nx-cloud bot commented Aug 27, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 162a445. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

Copy link

pkg-pr-new bot commented Aug 27, 2024

commit: 162a445

@tanstack/history

pnpm add https://pkg.pr.new/@tanstack/history@1790

@tanstack/react-cross-context

pnpm add https://pkg.pr.new/@tanstack/react-cross-context@1790

@tanstack/react-router

pnpm add https://pkg.pr.new/@tanstack/react-router@1790

@tanstack/react-router-with-query

pnpm add https://pkg.pr.new/@tanstack/react-router-with-query@1790

@tanstack/router-cli

pnpm add https://pkg.pr.new/@tanstack/router-cli@1790

@tanstack/router-devtools

pnpm add https://pkg.pr.new/@tanstack/router-devtools@1790

@tanstack/router-generator

pnpm add https://pkg.pr.new/@tanstack/router-generator@1790

@tanstack/router-plugin

pnpm add https://pkg.pr.new/@tanstack/router-plugin@1790

@tanstack/router-vite-plugin

pnpm add https://pkg.pr.new/@tanstack/router-vite-plugin@1790

@tanstack/start

pnpm add https://pkg.pr.new/@tanstack/start@1790

@tanstack/start-vite-plugin

pnpm add https://pkg.pr.new/@tanstack/start-vite-plugin@1790

Open in Stackblitz

More templates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants