-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
refactor(router-core): minor cleanup of loadMatches #4799
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
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 2bee3e9
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-with-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
...(this.state.pendingMatches ?? []), | ||
...this.state.matches, | ||
].find((d) => d.id === matchId) | ||
const find = (d: { id: string }) => d.id === matchId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const find = (d: { id: string }) => d.id === matchId | |
const findFn = (d: { id: string }) => d.id === matchId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know what I broke in this PR that I thought was basic, so I applied your proposed change in a separate PR that just touches that method #4824
@@ -2858,7 +2813,7 @@ export class RouterCore< | |||
: (route.options.gcTime ?? this.options.defaultGcTime)) ?? | |||
5 * 60 * 1000 | |||
|
|||
return !(d.status !== 'error' && Date.now() - d.updatedAt < gcTime) | |||
return d.status === 'error' && Date.now() - d.updatedAt >= gcTime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we break up one-liner into a couple of booleans? with variables like isError
and isElapsed
.
This PR is a minor cleanup of
router.ts
.some()
instead of.find()
when we only need a boolean resultupdateMatch
is never used, so don't return anythinggetMatch
doesn't need to create an array of all matchesloadMatches
route.options.beforeLoad
route.options.loader
And some minor "don't create an object if you don't need to".
A good goal would be to try and make
loadMatches
much lighter by updating the store less (fewer calls toupdateMatch
) as this can trigger a lot of synchronous code execution every time. But this PR only does minor cleanup because the big refactor is scary.