move prefetch logic out of useLink and into composables #288
Conversation
✅ Deploy Preview for kitbag-router ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Overall LGTM. I'd be fine with merging this as is. Comments are mostly nits, thoughts, suggestions. Everything minor.
I was approaching this as a single prefetching composition. Which might be preferred in the future once we get to prefetch strategies. But I like the separation because its easier to grok.
| const routeRef = ref(route) | ||
| const prefetchRef = ref(prefetch) |
There was a problem hiding this comment.
nit: using toValue inside the matchesToPrefetchComponents is preferred
| import { ResolvedRoute } from '@/types/resolved' | ||
| import { isAsyncComponent } from '@/utilities/components' | ||
|
|
||
| export function usePrefetchedComponent(route: MaybeRef<ResolvedRoute | undefined>, prefetch: MaybeRef<PrefetchConfigs | undefined>): void { |
There was a problem hiding this comment.
MaybeRefOrGetter is preferred over MaybeRef just because its more flexible.
| import { ResolvedRoute } from '@/types/resolved' | ||
|
|
||
| export type UsePrefetchedProps = { | ||
| prefetchProps: Ref<Record<string, unknown>>, |
There was a problem hiding this comment.
We probably don't need to expose this right?
|
|
||
| export type UsePrefetchedProps = { | ||
| prefetchProps: Ref<Record<string, unknown>>, | ||
| commitPrefetchedProps: () => void, |
There was a problem hiding this comment.
nit: kinda like how simple "commit" would be here.
| commitPrefetchedProps: () => void, | |
| commit: () => void, |
| commitPrefetchedProps: () => void, | ||
| } | ||
|
|
||
| export function usePrefetchedProps(route: MaybeRef<ResolvedRoute | undefined>, prefetch: MaybeRef<PrefetchConfigs | undefined>): UsePrefetchedProps { |
There was a problem hiding this comment.
Another spot to use MaybeRefOrGetter
One thing I did in my version of this was may this a single object so you could do this
usePrefetchedProps(() => ({
route: route.value,
routerPrefetch: router.prefetch,
linkPrefetch: options.prefetch
}))
Don't feel strongly, just felt nice when I was playing with it.
| const routeRef = ref(route) | ||
| const prefetchRef = ref(prefetch) | ||
| const store = usePropStore() | ||
| const prefetchProps = ref<Record<string, unknown>>({}) |
There was a problem hiding this comment.
this isn't reactive so we should just use a let here. Also, we're in a prefetch props composition. We could probably simplify some of these names.
| const prefetchProps = ref<Record<string, unknown>>({}) | |
| const props: Record<string, unknown> = {} |
There was a problem hiding this comment.
I see you are exposing it. I guess I wouldn't expose it and I wouldn't make it reactive. That would be my suggestion.
| const store = usePropStore() | ||
| const prefetchProps = ref<Record<string, unknown>>({}) | ||
|
|
||
| watch([routeRef, prefetchRef], ([route, prefetch]) => { |
There was a problem hiding this comment.
The single object works nicely here just because you end up with this
watch(option, ({ route, ...configs }) => {
...
})
| function commitPrefetchedProps(): void { | ||
| store.setPrefetchProps(prefetchProps.value) | ||
| } |
There was a problem hiding this comment.
I really like the "commit" idea ❤️ I struggled with what to call this.
this PR moves logic out of useLink.vue and into 2 new composables
usePrefetchedComponentandusePrefetchedProps. The main goal here is reducing complexity of useLink but could also be useful in the future if additional places might use prefetching logic.