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: add api for preloading route loaders #435

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

axwalker
Copy link
Sponsor

This stems from the discussion on enabling preloading of data loaders.

This is just the start of the approach and could mostly be complete nonsense! I just wanted to make sure I pushed where I'd got to so far before Monday in case you investigate this on your stream at all.

I've done some of the steps we discussed, specifically:

  1. Extract the required core logic of those beforeEach and beforeResolve guards into exported reusable functions
  2. Export a preloadData function somewhere pulling this all together for users to import and use

I don't know the actual location you'd want some of this stuff to live, I just wanted to get a proof of concept up and running first. If you visit the playground, you'll now see a 'preload' button in addition to 'go' button in the 'Navigate to' form. Clicking this will run what we have so far. A [name] route is where I've been playing with it so far. I can help adding tests later once I understand things more. This was just the easiest way for me to start playing and understanding.

At the moment, it does seemingly run the correct loader with the the correct route, but it still affects the isLoading status of the active loader on the current page if you're already on a [name] route.

I'm still in the very early days of understanding all the internals at play as someone who's just come from userland. I might need some assistance moving forward!

)
}

export async function preloadRoute(router: Router, route: RouteLocationRaw) {
Copy link
Sponsor Author

Choose a reason for hiding this comment

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

I assume this function wouldn't actually want to live here eventually but I just wanted to keep it close while I started investigating for simplicity.

}

export async function preloadRoute(router: Router, route: RouteLocationRaw) {
const _route = router.resolve(route)
Copy link
Sponsor Author

Choose a reason for hiding this comment

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

I guess there's a way to access the router without having to pass it in like this? useRouter didn't seem to do the trick.

Copy link
Owner

Choose a reason for hiding this comment

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

Not in a function like this. Passing the router instance is totally fine


export async function preloadRoute(router: Router, route: RouteLocationRaw) {
const _route = router.resolve(route)
await collectLoaders(router, _route)
Copy link
Sponsor Author

Choose a reason for hiding this comment

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

I realise the route types here don't match, but I'm still not sure how something goes from eg RouteLocationResolvedGeneric to RouteLocationNormalizedLoadedGeneric.

Copy link
Owner

Choose a reason for hiding this comment

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

The Loaded suffix here means that the components field in the route record is loaded and that all values are components that can be rendered. There is a function in vue-router loadRouteLocation() that allows to ensure this

@axwalker
Copy link
Sponsor Author

I still have a question about what we actually want the preloadData (or however we expose it) function to really do.

If someone ran preloadData and then 10 seconds later navigated to the route, should the loaders actually get run twice, or should the second time use the result of the first? I assume we'd want this to be fairly naive and just run it twice, and any benefits will come from caching implementations in the route itself, ie from vue-query or pinia-colada or similar.

@posva
Copy link
Owner

posva commented Jul 1, 2024

If someone ran preloadData and then 10 seconds later navigated to the route, should the loaders actually get run twice, or should the second time use the result of the first? I assume we'd want this to be fairly naive and just run it twice, and any benefits will come from caching implementations in the route itself, ie from vue-query or pinia-colada or similar.

Yeah, I would say preloading should be an optional part of the interfaces to be implemented by loaders. I still need to play around with ideas to find good ergonomics (.preload?.(), load(..., true), load(..., {preload: true}?). That way it offers a way for loaders to know where the load is coming from.

router,
effect,
isSSR,
selectNavigationResult = (results) => results[0]!.value,
Copy link
Owner

Choose a reason for hiding this comment

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

the default value shouldn't be needed here

router,
loaders,
app: router[APP_KEY],
effect: effectScope(),
Copy link
Owner

Choose a reason for hiding this comment

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

The effect must be the one created within the DataLoaderPlugin. It should probably be added to the router with a symbol like other properties

Copy link
Sponsor Author

Choose a reason for hiding this comment

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

Have done this now

app
// allows inject and provide APIs
.runWithContext(() =>
loader._.load(to as RouteLocationNormalizedLoaded, router)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm realizing I should be able to fix the need for this cast in Vue Router

Copy link
Owner

@posva posva left a comment

Choose a reason for hiding this comment

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

I think this is a good starting point: it lays out the working foundation. Next step would be start writing tests and thinking about edge cases (what happens if a navigation is triggered while a preload happen). Right now there is no way to distinguish a preload from a load but maybe it should exist.

If you want, you can add empty test cases with it.todo(...). I'm not sure if the tests will belong to the navigation-guard spec or the the define loader tests. Probably both. If not, I can take it from here.

@axwalker
Copy link
Sponsor Author

axwalker commented Jul 2, 2024

I've added a couple of tests to start with.

Right now there is no way to distinguish a preload from a load but maybe it should exist.

I can see this being useful. I'm not sure what API you want there, or what behaviour you'd want for each of the loader implementations right now so I haven't added any tests for the loaders yet. I guess there are a few scenarios:

  1. preloadRoute('/a') called -> preload resolves -> router.push('/a')
  2. preloadRoute('/a') called -> router.push('/a') -> preload resolves -> navigation resolves
  3. preloadRoute('/a') called -> router.push('/a') -> navigation resolves -> preload resolves
  4. preloadRoute('/a') called -> router.push('/b') -> ???

In which of these cases, if any, should the preloaded loaders be aborted? Should it be configurable at the point of eg defineBasicLoader(...)? In my own use cases, I'm using vue-query so I'll generally be happy for loader calls not getting aborted/deduped in any way because vue-query itself will handle any of that for me.

(note that personally I'm just using vue-query with defineBasicLoader right now because I wasn't sure of the state of the defineQueryLoader implementation. I'd be happy to help with any thoughts on defineQueryLoader separately at some point if you wish)

@posva
Copy link
Owner

posva commented Jul 2, 2024

regarding the tests, I would say that preloading should be reused if possible. If that's not possible, loaders should be aware of an existing preload to reuse it if they want.

Differently from a navigation, preloads shouldn't cancel each out.

defineQueryLoader() is still behind 😓 . I think it's better not to focus too much on it at the moment. I personally try to move the basic and colada loaders forward first, then I will see about others.

FYI it will take me some time to come back to this (1-2 weeks). I need to focus on other stuff for a while

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.

2 participants