prefetch page context

[usk94]

resolve #246

This PR introduces a feature to prefetch pageContext (specifically focusing on data) when users hover over a link.

To ensure efficient network usage, the prefetch operation includes a cooldown period to prevent it from being re-triggered too soon after the initial action. And if the user hovers over multiple links, pageContext fetched for previous links can be used instead of being discarded.

Users can configure this feature through +config.ts or by setting data-prefetch-page-context attributes directly on anchor tags.

[usk94]

@brillout I would appreciate some guidance on how to determine when the prefetch of pageContext is complete. While I have managed to fetch the context containing config, exportsAll, etc., when hovering over a link, I am unsure how to carry this data over to the rendering of the next page after navigation.

Any advice on this would be greatly helpful. Thank you in advance for your assistance.

[brillout]

@usk94 I will have a look and reply after I'm finished what I'm currently working on. ETA (beginning) of next week.

[usk94]

@brillout Thank you for your response; I await your further insights.

[brillout]

@usk94 Alright, let's do this!

Have a look at renderPageClientSide.ts, that's where the whole client-side rendering orchestration happens. This file has a globalObject so I think we can save the prefetched pageContext there. As for the prefetching itself, how about we use getPageContextFromHooks_isNotHydration() to do the pre-fetching?

Open question: do you think viewport prefetching makes sense for pageContext pre-fetching? This seems quite aggressive, maybe too aggressive. I'd be inclined to not even offer this setting :thinking:

[usk94]

@brillout

Have a look at renderPageClientSide.ts, that's where the whole client-side rendering orchestration happens. This file has a globalObject so I think we can save the prefetched pageContext there. As for the prefetching itself, how about we use getPageContextFromHooks_isNotHydration() to do the pre-fetching?

Thanks! I'll try it.

Open question: do you think viewport prefetching makes sense for pageContext pre-fetching? This seems quite aggressive, maybe too aggressive. I'd be inclined to not even offer this setting 🤔

Are you concerned that, for example, it might be too aggressive in terms of saturating the network with too many API requests?

For instance, in frameworks like Astro, when viewport prefetching is enabled, the priority of these requests is managed to be lower to mitigate potential network congestion. https://docs.astro.build/en/guides/prefetch/

But even with that consideration, I agree that it might be too aggressive. I think starting with hover prefetching would be sufficient.

[brillout]

todo: getPageContextFromHooks_isNotHydration return does not contains "urlOriginal", "Page". // maybe I need to merge with pageContext(props).

Yes, that's expected. It only fetches the pageContext from hooks, most notably onBeforeRender() and data() (but not onRenderClient()).

I was thinking whether we should also call onRenderClient() upon prefetching but that isn't trivial (it would need to render the next page in some kind of virtual DOM). So I'm thinking that only using getPageContextFromHooks_isNotHydration() is fine for now.

Let me know if you think getPageContextFromHooks_isNotHydration() doesn't fit, I can do some refactoring.

Are you concerned that, for example, it might be too aggressive in terms of saturating the network with too many API requests?

Yes, and also overloading the server (its database and logs).

But even with that consideration, I agree that it might be too aggressive. I think starting with hover prefetching would be sufficient.

:+1: We could also, eventually, add support for viewport prefetching for individual links that opt in using the data- attribute but I think we can skip that for now.

Astro

Indeed Astro seems to be using clever heuristics. If you want we can improve Vike's prefetching in a follow up PR after this one. I also have a couple of ideas for improving static asset prefetching.

[brillout]

@Boeing787 (sponsor who requested this feature) Do you have any special wishes? Your use case is that you want data to be prefetched when the user hovers a on link, correct?

[Boeing787]

@brillout correct, to load the pageContext

[usk94]

@brillout I think the PR is now ready for review. Could you please take a look?

Also, some CI tests has been failing since the second to last commit, and I'm unable to pinpoint the issue. If possible, could you provide some insight into what might be going wrong?

[brillout]

Hi Yusaku, thank you.

Looks good at first sight, let me have a closer look.

some CI tests has been failing since the second to last commit

That seems surprising indeed. Let me dig.

[brillout]

As for the failing CI, I think we can have a look at it later.

[usk94]

@brillout Thank you for your review! Except where I responded to your comments, I have updated based on your feedback. Could you please check it again?

[brillout]

@brillout Thank you for your review! Except where I responded to your comments, I have updated based on your feedback. Could you please check it again?

:+1: I can re-review after we resolved the two pending conversations, but let me know if you want me to re-review before that. Looking forward to it!

[usk94]

@brillout I have updated on all your feedback. May I ask for a re-review?

[brillout]

@usk94 Neat. I will, tentatively today.

[brillout]

I've started, I'll finish tomorrow.

[brillout]

https://github.com/vikejs/vike/pull/1617/commits/b0f2579739c279d1ef41f85ebaaecf1c0b5f339b fixes the CI.

One (/ the only?) reason is that we need to use the URL instead of _pageId as cache key, because of parameterized routes.

[phonzammi]

@brillout, Could the issue be that lastPrefetch and'expire might be undefined?

[brillout]

@phonzammi Maybe, I didn’t dig into that part yet 👀

[usk94]

@brillout

One (/ the only?) reason is that we need to use the URL instead of _pageId as cache key, because of parameterized routes.

Thanks! CI passes now.

[usk94]

That's surprising indeed. I suggest we have a look at it again after we're happy with the rest. Marking as resolved in the meantime.

And when I switched from process.env.NODE_ENV back to import.meta.env.DEV at this time, there were no errors successfully.

[brillout]

👍 I’ll review again today/tomorrow.

[brillout]

I added some comments. I'll resuming reviewing after the comments are resolved.

[usk94]

@brillout I fixed except for the point to which I replied. However, the CI is down again, could you provide some insight into what might be going wrong?

[brillout]

However, the CI is down again, could you provide some insight into what might be going wrong?

I think the failing tests are easy to reproduce locally? In general, the expectation is that PR authors dig into why their changes make the CI fail themselves. That said, let me know if the test logs aren't helpful (e.g. if the logs shown by @brillout/test-e2e aren't helpful).

[usk94]

@brillout You are right. I'll do some more research.

[brillout]

@brillout You are right. I'll do some more research.

But feel free to let me know if after having digged you think it's mostly unrelated to your changes.

[usk94]

memo:

• A part of objectAssign was restored so that all but Unit tests E2E could pass.
• It fails because timestamp2 and timestamp6 match in test/misc/testRun.ts#history.pushState()

[usk94]

@brillout Sorry, it seems that my change was the cause. Adding lastPrefetchTime as an item in prefetchedPageContexts is causing the CI(Unit tests E2E) to fail.

I made a slight adjustments to handle whether lastPrefetchedPageContext.lastPrefetchTime(the result of find on prefetchedPageContexts) is undefined or not, but the CI is still failing. I'm not sure what is preventing the success of E2E testing, and I apologize for that, but would it be okay to leave this part as it is for now?

[brillout]

There are also other tests that are failing, have you looked at these ones? The history.pushState() test is a fairly complex test; instead you may want to have a look at why the boilerplate test is failing which is a much easier test file.

Keep me updated. I can also have a look at why the CI is failing, just let me know.