Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add useNavigate #123

Closed
wants to merge 7 commits into from
Closed

add useNavigate #123

wants to merge 7 commits into from

Conversation

nitedani
Copy link
Member

@nitedani nitedani commented Jun 28, 2024

Alternative implemenation: #124.

@nitedani
Copy link
Member Author

nitedani commented Jun 28, 2024

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 nitedani marked this pull request as ready for review June 28, 2024 01:13
@brillout
Copy link
Member

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
Copy link
Member

I just thought a little more about it.

When I created vikejs/vike#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 vikejs/vike#1707.

Let me know if you disagree.

Thank you for digging and the new insights 👀

@nitedani
Copy link
Member Author

nitedani commented Jun 30, 2024

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
Copy link
Member Author

nitedani commented Jun 30, 2024

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
Copy link
Member

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
Copy link
Member

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

@brillout brillout closed this Aug 19, 2024
@brillout
Copy link
Member

brillout commented Nov 11, 2024

Workaround and further rationale why, so far, I believe we should deprioritize this: 96da471.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants