vikejs / vike

🔨 Flexible, lean, community-driven, dependable, fast Vite-based frontend framework.
https://vike.dev
MIT License
4.4k stars 353 forks source link

Client-only guard() hook for pre-rendered pages #1916

Open lfos opened 1 month ago

lfos commented 1 month ago

Description

To reproduce, add a simple guard() hook and configure guard() to run client-side only:

export async function guard(pageContext) {
  console.log("guard");
}

The hook is only executed when using client-side navigation, and not on initial page load.

After reading the first few comments in #1600, I thought this was a known limitation, but after reading the whole discussion and re-reading https://vike.dev/guard, I am no longer sure this is known/intended behavior. (If it is, I believe the documentation should be further improved.)

lfos commented 1 month ago

For context, this is on an SSG site, using (or attempting to use) the hook to redirect/render different pages under certain preconditions.

brillout commented 1 month ago

under certain preconditions

Can you elaborate on that?

lfos commented 1 month ago

under certain preconditions

Can you elaborate on that?

Authentication with SSG is an example that was already mentioned in #1600, but there are other similar scenarios, such as having a whole part/section of a website that is only available after the user has taken specific action (e.g., added some data, made a purchase, ...) Essentially anything you might imagine as reasonable use cases for guard() on a SSR page.

lfos commented 1 month ago

@brillout If this is treated as "enhancement" rather than a bug, would it be possible to at least make the documentation very clear about the current state in the meantime? For example, https://vike.dev/guard#environment mentions that guard() can be configured to run on the client side, and the section on hook execution order here seems to explicitly state that client-side configuration means the hooks are executed on both both initial render and navigation. It took me quite some time and manual debugging to understand the current state/limitations and inconsistency with documentation.

brillout commented 1 month ago

IIRC the guard() hook is actually called, but at pre-render time. It indeed doesn't suit authentication use cases. I'll think about this once I'm done with my current priorities (ETA tomorrow / this week).

brillout commented 1 month ago

I agree that for pre-rendered pages guard() should be called only on the client-side. I'm labeling this as highest-prio since it's a blocker for users (there is a workaround so it actually isn't a blocker, but we should still make guard() work as users expect).

The Vike's CLI (https://github.com/vikejs/vike/issues/1340 and https://github.com/vikejs/vike/issues/1341) is a blocker for implementing this, so I'm afraid this will take a little while until I can proceed with the implementation.

After I'm done with my work on improving Vike's website and docs, I'll be working on these two blocking tickets and then finally this ticket.

brillout commented 1 month ago

In the meantime, you can try to use throw render() / throw redirect() in one of the many client-side hooks such as onHydrationEnd() and/or onBeforeRenderClient().

lfos commented 1 month ago

I agree that for pre-rendered pages guard() should be called only on the client-side. I'm labeling this as highest-prio since there is a clear design decision ~and it's a blocker for users~ (there could be a workaround so maybe it isn't a blocker).

Great, thank you!

In the meantime, you can try to use throw render() / throw redirect() in one of the many client-side hooks such as onHydrationEnd() and/or onBeforeRenderClient().

I don't think either of these work, unfortunately. onBeforeRenderClient() seems to have the same problem as guard(): it is not called client-side on the first page load. When throwing render() or redirect() from onHydrationEnd() , nothing happens; maybe it is too late at that point?

brillout commented 1 month ago

I'll have a look at why it doesn't work. ETA this week.

brillout commented 1 month ago

(New ETA this week.)

brillout commented 1 month ago

onBeforeRenderClient() seems to have the same problem as guard(): it is not called client-side on the first page load.

No, onRenderClient() (and thus onBeforeRenderClient()) is always called including for the first page.

When throwing render() or redirect() from onHydrationEnd() , nothing happens

I just tried calling throw redirect() in onHydrationEnd() and it's working. Reproduction welcome.

I also tried inside onBeforeRenderClient() and it's working as well. Reproduction for this welcome as well.

lfos commented 4 weeks ago

onBeforeRenderClient() seems to have the same problem as guard(): it is not called client-side on the first page load.

No, onRenderClient() (and thus onBeforeRenderClient()) is always called including for the first page.

When throwing render() or redirect() from onHydrationEnd() , nothing happens

I just tried calling throw redirect() in onHydrationEnd() and it's working. Reproduction welcome.

I also tried inside onBeforeRenderClient() and it's working as well. Reproduction for this welcome as well.

Thanks for investigating! Did you also test this with SSG pages generated with npm run build/npx vite build (and not just npm run dev/npx vite)? If you can't reproduce, I'll try to put together a minimal example over the coming days.

brillout commented 4 weeks ago

Yes

On Mon, Oct 28, 2024 at 11:59 AM Lukas Fleischer @.***> wrote:

onBeforeRenderClient() seems to have the same problem as guard(): it is not called client-side on the first page load.

No, onRenderClient() (and thus onBeforeRenderClient()) is always called including for the first page.

When throwing render() or redirect() from onHydrationEnd() , nothing happens

I just tried calling throw redirect() in onHydrationEnd() and it's working. Reproduction welcome.

I also tried inside onBeforeRenderClient() and it's working as well. Reproduction for this welcome as well.

Thanks for investigating! Did you also test this with SSG pages generated with npm run build/npx vite build (and not just npm run dev/npx vite)? If you can't reproduce, I'll try to put together a minimal example over the coming days.

— Reply to this email directly, view it on GitHub https://github.com/vikejs/vike/issues/1916#issuecomment-2441261043, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHVQRXDRPOLQO7DTIYPVMTZ5YKKLAVCNFSM6AAAAABPRKL3NKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBRGI3DCMBUGM . You are receiving this because you were mentioned.Message ID: @.***>

lfos commented 3 weeks ago

It looks like you're right; I had an independent bug in my code. After fixing it, onBeforeRenderClient() is triggered correctly.

The last part that's missing for this to be a viable workaround is a way to set pageContext. While onBeforeRenderClient() seems to differ from onBeforeRender() in that we cannot return an (updated) page context (why?), it seems to be possible to just overwrite/add properties directly. Can you confirm this is supported, in the sense that there shouldn't be any unexpected side effects? Thanks again for all the help!

brillout commented 3 weeks ago

Yes, see https://vike.dev/pageContext#custom and e5dc5c44cbba8997f14016e3b23494dd7782afce.

lfos commented 2 weeks ago

Awesome, thanks for confirming! Still looking forward to having full client-side guard() hook support.