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

onBeforeRouteEnter not triggered if coming from reject route #244

Open
stackoverfloweth opened this issue Aug 15, 2024 · 4 comments
Open

Comments

@stackoverfloweth
Copy link
Contributor

I added a onBeforeRouteEnter hook to my route

const profile = createRoute({
	name: 'profile',
	path: '/profile',
	component: defineAsyncComponent(() => import('../components/ProfilePage.vue')),
	onBeforeRouteEnter: (to, { reject }) => {
		if (!activeUser.value) {
			reject('AuthNeeded')
		}
	},
})

works totally as expected the first time, if I try navigating to /profile without an active user the rejection happens, is handled by sending me to rejection route (doesn't matter if I use built in rejection component or custom). However, if I click a router-link again to go to /profile it seems to totally bypass this onBeforeRouteEnter hook.

@pleek91
Copy link
Contributor

pleek91 commented Aug 15, 2024

@stackoverfloweth is this when navigating from the rejection back to profile? Or just anytime you try and go to profile after hitting a rejection?

@stackoverfloweth
Copy link
Contributor Author

stackoverfloweth commented Aug 21, 2024

@pleek91 an update on this
I traced the issue to getBeforeRouteHooksFromRoutes, which takes to and from and returns all the hooks.

The first time I enter my route with this hook I see a hook in onBeforeRouteEnter where I'd expect
image

The second time I enter my route the onBeforeRouteEnter is empty
image

This line has a condition isRouteEnter, that is almost certainly the issue but I don't understand what it's trying to do assert.

I made a branch in our router-preview repo setup to reproduce
https://github.com/kitbagjs/router-preview/compare/issue-244-repro?expand=1

@pleek91
Copy link
Contributor

pleek91 commented Nov 30, 2024

@stackoverfloweth I wrote a test that I think reproduces what you're talking about there. Responding here mostly so I can remember what I found.

The issue is you didn't actually change routes. You navigated to /profile which triggered a rejection. But you're still on that same route. So when you push to /profile again its a route update not a route enter. You can see in this test that the update hook is called.

So I think its working as designed. But maybe not working as it needs to? I think the logic for enter vs update is correct. But it doesn't maybe match up with developers expectations when combined with rejections.

test('test hooks', async () => {
  const enter = vi.fn()
  const update = vi.fn()
  let activeUser = false

  const profile = createRoute({
    name: 'profile',
    path: '/profile',
    component: { template: 'Profile' },
    onBeforeRouteEnter: (to, { reject }) => {
      enter()
      if (!activeUser) {
        reject('NotFound')
      }
    },
    onBeforeRouteUpdate: () => {
      update()
    },
  })

  const router = createRouter([profile], {
    initialUrl: '/profile',
  })

  await router.start()

  const root = {
    template: '<RouterView/>',
  }

  const app = mount(root, {
    global: {
      plugins: [router],
    },
  })

  expect(app.text()).toBe('NotFound')
  expect(update).toHaveBeenCalledTimes(0)
  expect(enter).toHaveBeenCalledTimes(1)
  
  activeUser = true

  await router.push('/profile')

  expect(app.text()).toBe('Profile')
  expect(update).toHaveBeenCalledTimes(1)

  // this fails because the hook wasn't run a second time
  expect(hook).toHaveBeenCalledTimes(2) 
})

@stackoverfloweth
Copy link
Contributor Author

@pleek91 interesting... that definitely makes sense now. I like that rejections keep you on the route but I'm concerned that devs will make the same mistake I did assuming that because it feels like they're on a rejection route (not found) that they can push to profile and trigger route enter.

I'll continue to give this some thought but as of right now I'm tempted to treat rejection routes like separate routes and abandon the idea of just fix the error and call refresh. That's not intuitive and breaks hooks. Technically we have rejection routes, I'm wondering if we should just use those as the current route.

For the use case of auth needed and returning to the route they were trying to access, maybe we can do something else to still create a solid devex 🤔

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

No branches or pull requests

2 participants