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.

Sign up for GitHub

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

Jump to bottom

feat: provide location in onEnter/onStay/onLeave hooks #3622

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

feat: provide location in onEnter/onStay/onLeave hooks #3622

wants to merge 4 commits into from

Conversation

jprosevear
Copy link
Contributor

@jprosevear jprosevear commented Mar 1, 2025 •
edited
Loading

@nx-cloud Nx Cloud
Copy link

nx-cloud bot commented Mar 1, 2025 •
edited
Loading

View your CI Pipeline Execution ↗ for commit 406cec6.

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ❌ Failed 1m 39s View ↗
nx run-many --target=build --exclude=examples/*... ❌ Failed 4s View ↗

☁️ Nx Cloud last updated this comment at 2025-03-04 21:07:53 UTC

@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Mar 1, 2025 •
edited
Loading

Open in Stackblitz

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/arktype-adapter@3622

@tanstack/create-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/create-router@3622

@tanstack/create-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/create-start@3622

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/directive-functions-plugin@3622

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/eslint-plugin-router@3622

@tanstack/history

npm i https://pkg.pr.new/TanStack/router/@tanstack/history@3622

@tanstack/react-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router@3622

@tanstack/react-router-with-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-with-query@3622

@tanstack/react-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start@3622

@tanstack/react-start-api-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-api-routes@3622

@tanstack/react-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-client@3622

@tanstack/react-start-config

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-config@3622

@tanstack/react-start-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-plugin@3622

@tanstack/react-start-router-manifest

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-router-manifest@3622

@tanstack/react-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server@3622

@tanstack/react-start-server-functions-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server-functions-client@3622

@tanstack/react-start-server-functions-fetcher

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server-functions-fetcher@3622

@tanstack/react-start-server-functions-handler

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server-functions-handler@3622

@tanstack/react-start-server-functions-ssr

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server-functions-ssr@3622

@tanstack/router-cli

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-cli@3622

@tanstack/router-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-core@3622

@tanstack/router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools@3622

@tanstack/router-generator

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-generator@3622

@tanstack/router-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-plugin@3622

@tanstack/router-utils

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-utils@3622

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-vite-plugin@3622

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/server-functions-plugin@3622

@tanstack/solid-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router@3622

@tanstack/solid-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start@3622

@tanstack/solid-start-api-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-api-routes@3622

@tanstack/solid-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-client@3622

@tanstack/solid-start-config

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-config@3622

@tanstack/solid-start-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-plugin@3622

@tanstack/solid-start-router-manifest

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-router-manifest@3622

@tanstack/solid-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server@3622

@tanstack/solid-start-server-functions-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server-functions-client@3622

@tanstack/solid-start-server-functions-fetcher

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server-functions-fetcher@3622

@tanstack/solid-start-server-functions-handler

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server-functions-handler@3622

@tanstack/solid-start-server-functions-ssr

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server-functions-ssr@3622

@tanstack/start

npm i https://pkg.pr.new/TanStack/router/@tanstack/start@3622

@tanstack/start-config

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-config@3622

@tanstack/start-server-functions-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-functions-server@3622

@tanstack/valibot-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/valibot-adapter@3622

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/virtual-file-routes@3622

@tanstack/zod-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/zod-adapter@3622

commit: 9d71302

@jprosevear jprosevear force-pushed the feat/location-in-hooks branch 2 times, most recently from a0d83cb to e6aef99 Compare March 1, 2025 19:20
@github-actions github-actions bot added the documentation Everything documentation related label Mar 1, 2025
@jprosevear jprosevear force-pushed the feat/location-in-hooks branch from 017c52c to e0dae9b Compare March 1, 2025 19:47
schiller-manuel
schiller-manuel reviewed Mar 1, 2025
View reviewed changes
packages/solid-router/tests/route.test.tsx
@@ -191,6 +191,27 @@ describe('onEnter event', () => {

expect(fn).toHaveBeenCalledWith({ foo: 'bar' })
})

it('should have location defined in router.load()', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

rather should have location defined in onEnter?

please also add a test for onStay and onLeave

Copy link
Contributor

Choose a reason for hiding this comment

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

also applies to react-router of course

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was just copying the style from above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its in an onEnter describe block

schiller-manuel
schiller-manuel reviewed Mar 1, 2025
View reviewed changes
packages/router-core/src/route.ts Outdated
@@ -865,6 +865,7 @@ export interface UpdatableRouteOptions<
>,
TLoaderDeps
>,
{ location }: { location: ParsedLocation<{}> },
Copy link
Contributor

@schiller-manuel schiller-manuel Mar 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

we should be able to supply the search type here (same for the others as well)

probably makes sense to create a type OnMatchOptions<TSearch> = {location:ParsedLocation<TSearch>

and use this for all 3 methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think this additional typing is a little out of my immediate grasp - can you give some more guidance

Copy link
Contributor

Choose a reason for hiding this comment

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

before we do that, what is the semantic of location in each of these hooks?

I think in case of onLeave location is anything BUT the match, so we cannot statically type the search here.
whereas for the other hooks, the route's search is present in location

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its the location that will resolve (assuming nothing thrown) - played with current / next like the useBlocker stuff but decided on this approach.

@schiller-manuel schiller-manuel force-pushed the feat/location-in-hooks branch from 9d71302 to e285bbe Compare March 4, 2025 21:00
@schiller-manuel
typing search
879b53b 
wrong but as a starting point
@schiller-manuel schiller-manuel force-pushed the feat/location-in-hooks branch from 768968a to 879b53b Compare March 4, 2025 21:04
@jprosevear
Copy link
Contributor Author

@schiller-manuel how do you want to reconcile this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schiller-manuel schiller-manuel schiller-manuel left review comments

Assignees
No one assigned
Labels
documentation Everything documentation related package: react-router package: router-core package: solid-router
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jprosevear @schiller-manuel