vikejs / vike-react

🔨 React integration for Vike
https://vike.dev/vike-react
MIT License
94 stars 15 forks source link

add useNavigate #123

Closed nitedani closed 2 weeks ago

nitedani commented 2 months ago

Alternative implemenation: https://github.com/vikejs/vike-react/pull/124.

nitedani commented 2 months ago

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

On the server:

On the client: uses vike/client/router

Not sure about the folder structure and the types

brillout commented 2 months ago

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 commented 2 months ago

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:

nitedani commented 2 months ago

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 commented 2 months ago

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");
  }
}
brillout commented 2 months ago

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 commented 2 weeks ago

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