vike-react icon indicating copy to clipboard operation
vike-react copied to clipboard

add useNavigate

Open nitedani opened this issue 1 year ago • 6 comments

nitedani avatar Jun 28 '24 00:06 nitedani

Different implementation for server and client: node/hooks/useNavigate client/hooks/useNavigate

On the server:

  • If the headers are already sent(assuming streaming is enabled), the injected navigation script sets window.location.href
  • If the headers are not sent yet, the location can be sent in the headers after aborting on the server

On the client: uses vike/client/router

Not sure about the folder structure and the types

nitedani avatar Jun 28 '24 00:06 nitedani

Neat, and indeed we need some special handling for HTML streaming.

Let me know when the PR is finished and I'll have a closer look at it.

brillout avatar Jun 28 '24 05:06 brillout

I just thought a little more about it.

When I created https://github.com/vikejs/vike/issues/1707 I didn't think of the extra care HTML streaming requires for it. So I'm questioning whether it's worth it to work on this.

~~I wonder: is there a real use case for using throw redirect() inside React components?~~ Actually there is: when fetching some data /product/123 inside a component but no product 123 exists and the user should be redirected to /product/new. But, instead of using throw redirect(), the user can still render <ProductComponent> while showing No product '123' found, please go to /product/new which I'd argue is a more idiomatic approach in the context of HTML streaming.

(An idea: we could build that streaming logic into Vike core, so that users can directly use throw redirect() instead of using useNavigation(). But it would complexify Vike core a bit, so I'm also relunctant to do that. FYI Vike core knows about streams provided by react-streaming and already uses its injectToStream feature.)

So I'm thinking maybe we should postpone working on this until users stumble upon valid/important use cases for it.

I went ahead a deprioritized https://github.com/vikejs/vike/issues/1707.

Let me know if you disagree.

Thank you for digging and the new insights :eyes:

brillout avatar Jun 28 '24 06:06 brillout

An idea: we could build that streaming logic into Vike core, so that users can directly use throw redirect() instead of using useNavigation(). But it would complexify Vike core a bit, so I'm also relunctant to do that.

Yes, we would need to access react-streaming's boundary error from inside vike. Then maybe do something similar for solid, vue, etc.. Though I don't know how those handle errors during streaming. It could be a lot of work.. I don't think it's worth to dig into it right now, maybe later. Maybe we can add support for react first, if people want it.

nitedani avatar Jun 30 '24 04:06 nitedani

Actually there is: when fetching some data /product/123 inside a component but no product 123 exists and the user should be redirected to /product/new.

Maybe something like this? I know this can go in pages/login/+guard.ts, but is it better? I think guard is good too, but need to expose queryClient on pageContext in vike-react-query.

import { getProfile } from "#root/src/components/auth/auth.telefunc";

export function Login(){
  const navigate = useNavigate();
  const { data: profile } = useSuspenseQuery({ queryFn: getProfile, queryKey: ["profile"] });
  if (profile) {
    navigate("/home");
    return;
  }
 }

VS

// pages/login/+guard.ts
import { getProfile } from "#root/src/components/auth/auth.telefunc";
import { redirect } from "vike/abort";

export async function guard(pageContext) {
  // load profile in the query cache, to be used in react components
  const profile = await pageContext.queryClient.fetchQuery({ queryFn: getProfile, queryKey: ["profile"] });
  if (profile) {
    throw redirect("/home");
  }
}

nitedani avatar Jun 30 '24 04:06 nitedani

In general, I share the sentiment that defining logic on a component-level is nice.

But, actually for authentication, I ain't sure how natural that is. I don't know, but I feel like users are more inclined to think in terms of "these pages need authentication" instead of "these components need authentication". Actually, after thinking more about it, I think it's more natural to think on a page-level because any authentication requirement affects the whole page.

So I guess we can agree that being able to use throw render() / throw redirect() is a nice-to-have but not worth working on at the moment.

brillout avatar Jun 30 '24 11:06 brillout

Let's re-open once we work on this again.

brillout avatar Aug 19 '24 13:08 brillout

Workaround and further rationale why, so far, I believe we should deprioritize this: https://github.com/vikejs/vike-react/commit/96da47144205715e6b76e727bc591da9f71cc2d3.

brillout avatar Nov 11 '24 11:11 brillout