prefetch page context #1617

usk94 · 2024-04-22T23:40:26Z

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 · 2024-04-25T23:19:52Z

@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 · 2024-04-28T16:02:36Z

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

usk94 · 2024-04-28T16:15:07Z

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

brillout · 2024-05-12T12:55:49Z

@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 🤔

usk94 · 2024-05-14T00:34:35Z

@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 · 2024-05-14T08:05:46Z

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.

👍 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 · 2024-05-14T08:06:21Z

@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 · 2024-05-14T16:51:25Z

@brillout correct, to load the pageContext

…ntext._pageId` are mismatched

brillout

Overall this is going in the right direction 🚀 I made a lot of comments and a couple of new thoughts occurred to me in the process. Let me know what you think. Looking forward to it 👀

brillout · 2024-05-20T08:06:48Z

vike/client/client-routing-runtime/prefetch.ts

@@ -1,5 +1,6 @@
 export { prefetch }
 export { addLinkPrefetchHandlers }
+export { PrefetchedPageContext }


We can use export type { PrefetchedPageContext } to make it a little bit clearer that it's a type.

brillout · 2024-05-20T08:07:46Z

vike/client/client-routing-runtime/renderPageClientSide.ts

@@ -48,6 +48,10 @@ const globalObject = getGlobalObject<{
  previousPageContext?: { _pageId: string } & PageContextExports
 }>('renderPageClientSide.ts', { renderCounter: 0 })

+const globalObjectForPrefetchedPageContext = getGlobalObject<{


Let's re-use the globalObject above.

brillout · 2024-05-20T08:11:58Z

vike/client/client-routing-runtime/renderPageClientSide.ts

@@ -48,6 +48,10 @@ const globalObject = getGlobalObject<{
  previousPageContext?: { _pageId: string } & PageContextExports
 }>('renderPageClientSide.ts', { renderCounter: 0 })

+const globalObjectForPrefetchedPageContext = getGlobalObject<{
+  prefetchedPageContext?: PrefetchedPageContext
+}>('prefetch.ts', {})


Ah, I see, you basically use getGlobalObject(..., 'prefetch.ts') to share information between modules. Interesting usage.

Or how about we export a new function getPrefetchedPageContext() in prefetchts.ts instead? I've a slight preference for this as it makes the code flow more explicit.

brillout · 2024-05-20T08:18:43Z

vike/client/client-routing-runtime/renderPageClientSide.ts

+        if (
+          prefetchedPageContext?.pageContextFromHooks &&
+          '_pageId' in prefetchedPageContext.pageContextFromHooks &&
+          prefetchedPageContext.pageContextFromHooks._pageId === pageContext._pageId


Instead, how about we make prefetchedPageContext hold one pageContext per pageId?

Sorry, I'm not sure I understand your point. May I ask for more details?

I think we can save multiple pageContext objects, one per pageId. In other words globalObject.prefetchedPageContext would be a Record<string, PrefetchedPageContext> where string is the pageId. The idea is that if the user hovers over multiple links, the pageContext fetched for previous links can be used instead of being discarded.

I understood, thank you!

The idea is that if the user hovers over multiple links, the pageContext fetched for previous links can be used instead of being discarded.

Then, I have implemented this with { pageId: string; prefetchedPageContext: PrefetchedPageContext }[] rather than Record<string, PrefetchedPageContext>(because I struggled to fix type errors in getPageContextFromHooks.ts)

👍 We can polish such small details later.

brillout · 2024-05-20T08:21:48Z

vike/client/client-routing-runtime/renderPageClientSide.ts

@@ -203,9 +207,18 @@ async function renderPageClientSide(renderArgs: RenderArgs): Promise<void> {
      // Render page view
      await renderPageView(pageContext)
    } else {
-      let res: Awaited<ReturnType<typeof getPageContextFromHooks_isNotHydration>>
+      let res: Awaited<ReturnType<typeof getPageContextFromHooks_isNotHydration>> | PrefetchedPageContext
      try {


I thinks we can move the new code outside of the try-catch block?

vike/client/client-routing-runtime/prefetch/getPrefetchSettings.ts

brillout · 2024-05-20T09:37:40Z

vike/client/client-routing-runtime/prefetch/getPrefetchSettings.ts

@@ -109,3 +152,44 @@ function getPrefetchAttribute(linkTag: HTMLElement): PrefetchStaticAssets | null

  assert(false)
 }
+
+function getPrefetchPageContextAttribute(linkTag: HTMLElement): PrefetchPageContext | null {
+  const whenAttr = linkTag.getAttribute('data-prefetch-page-context-when')


How about data-prefetch-page-context instead of data-prefetch-page-context-when?

brillout · 2024-05-20T09:42:58Z

vike/client/client-routing-runtime/prefetch.ts

+async function prefetchContextIfPossible(url: string, expire: number | undefined): Promise<void> {
+  const now = Date.now()
+  const lastPrefetch = globalObject?.lastPrefetchTime?.get(url)
+  if (lastPrefetch && expire && now - lastPrefetch < expire) {


How about some default expire value? E.g. 5 seconds or maybe 30 seconds? I'm not sure.

I wonder whether expire should also apply to prefetch() 🤔 WDYT?

I wonder whether expire should also apply to prefetch() 🤔 WDYT?

I think it would be reasonable to exclude the expire logic, as it is expected to be triggered less frequently than hover.

How about some default expire value? E.g. 5 seconds or maybe 30 seconds? I'm not sure.

For now, I set it up in 5 seconds.

brillout · 2024-05-20T09:48:12Z

vike/client/client-routing-runtime/prefetch/getPrefetchSettings.ts

+      return { when: 'hover', expire: parseInt(expireAttr, 10) }
+    }
+
+    assertUsage(false, `data-prefetch-page-context-expire has value "${expireAttr}" but it should instead be number`)


There is some code repetition here. See code above which is a duplicate. This is client-side code so it's important to keep it compact.

brillout · 2024-05-20T09:51:22Z

vike/client/client-routing-runtime/prefetch/getPrefetchSettings.ts

+  if ('prefetchPageContext' in pageContext.exports) {
+    const { prefetchPageContext } = pageContext.exports
+
+    const wrongUsageMsg = `prefetchPageContext should be an object with 'when' and 'expire' properties. 'when' should be false, 'hover', and 'expire' should be a number`


Actually, thinking about it: how about we make the setting simply boolean | number instead?

brillout · 2024-05-20T09:58:37Z

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

…n modules

usk94 · 2024-05-20T23:54:18Z

@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 · 2024-05-21T14:29:54Z

@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?

👍 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 · 2024-05-22T14:10:54Z

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

brillout · 2024-05-22T14:13:01Z

@usk94 Neat. I will, tentatively today.

brillout · 2024-05-24T16:17:49Z

I've started, I'll finish tomorrow.

This reverts commit b0f2579.

brillout · 2024-05-25T14:34:14Z

b0f2579 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 · 2024-05-25T15:35:39Z

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

brillout · 2024-05-25T19:01:28Z

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

This reverts commit 49ac71e.

usk94 · 2024-05-26T08:40:43Z

@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 added 10 commits April 23, 2024 08:36

fix type. rename type file.

1850c01

create getPrefetchPageContext

2d32a46

wip for prefetch page context

9c640b2

add getPrefetchPageContextAttribute

023aeb5

fix for getPrefetchPageContext

2ffe8ef

fix prefetchPageContext arg type

50fa4dc

for config definition

9afba77

for when lowercase

c15164b

objectAssign for PageContextExports & others

7dc7a60

add properties to objectAssign

7494cc5

fix type

6cf6c2d

use getPageContextFromHooks_isNotHydration

29f3fd5

usk94 added 10 commits May 16, 2024 10:02

pass perfetched pageContextFromHooks to renderPageClientSide

8aca3e6

fix type

5b72769

remove viewport pattern

8121b93

remove unuse

663745f

Merge remote-tracking branch 'origin/main' into prefetch-page-context

5333dd7

use process.env.NODE_ENV

86a7ea5

move objectAssign into prefetchPageContext

000ed99

just returning in prefetchPageContext catch for now

b094f1c

fix 500 error when prefetchedPageContext._pageId and normal `pageCo…

ea9eadf

…ntext._pageId` are mismatched

do not run another prefetch for specified seconds

077d865

brillout reviewed May 20, 2024

View reviewed changes

usk94 added 8 commits May 21, 2024 00:14

specify type. mv PrefetchPageContext.

9481624

carry route() one layer up

8d0bc34

add ImportString

e9cfb91

revert rename

74d05b1

move the new code outside of the try-catch

e0a6165

polish getPrefetchSettings.ts

1721bbe

export getPrefetchedPageContext instead of sharing information betwee…

c3bd12b

…n modules

check whether pageContext expired in renderPageClientSide too

e9fb26b

usk94 marked this pull request as ready for review May 21, 2024 07:08

usk94 added 2 commits May 22, 2024 09:15

split getPageContextFromHooks_isNotHydration into two parts

0135883

for saving multiple pageContexts

4a93764

usk94 requested a review from brillout May 22, 2024 14:11

brillout added 4 commits May 25, 2024 15:43

fix split

7ba5b3e

further fix split

a87faa2

debug

b0f2579

Revert "debug"

c2cbd03

This reverts commit b0f2579.

usk94 added 3 commits May 26, 2024 10:28

use plain object instead of Map for lastPrefetchTime

49ac71e

Revert "use plain object instead of Map for lastPrefetchTime"

cf6db62

This reverts commit 49ac71e.

use urlOriginal instead of pageId

f64747c

prefetch page context #1617

Are you sure you want to change the base?

prefetch page context #1617

Conversation

usk94 commented Apr 22, 2024 • edited

usk94 commented Apr 25, 2024

brillout commented Apr 28, 2024

usk94 commented Apr 28, 2024

brillout commented May 12, 2024

usk94 commented May 14, 2024 • edited

brillout commented May 14, 2024

brillout commented May 14, 2024

Boeing787 commented May 14, 2024

brillout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brillout commented May 20, 2024

usk94 commented May 20, 2024

brillout commented May 21, 2024

usk94 commented May 22, 2024

brillout commented May 22, 2024

brillout commented May 24, 2024

brillout commented May 25, 2024

phonzammi commented May 25, 2024 • edited

brillout commented May 25, 2024

usk94 commented May 26, 2024

usk94 commented Apr 22, 2024 •

edited

usk94 commented May 14, 2024 •

edited

phonzammi commented May 25, 2024 •

edited