vikejs / vike

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

Better support for browser history navigation on the client #1169

Closed thomasjm closed 11 months ago

thomasjm commented 11 months ago

Description

I heard you were working on V1 so I was hoping to get this issue on your radar!

I recently migrated a React app to Vike that made heavy use of HTML history calls like history.pushState. I ran into the problem that it was difficult to reproduce the history stuff using Vike's API.

In the past, I've used redux-first-history, which does a good job consolidating router state into the Redux store, so the app has a single source of truth.

Here are the problems as I see it:

Hope this feedback helps! Thanks for the awesome project.

brillout commented 11 months ago

Thanks for creating this feature request. It's always helpful to know what users want.

consolidating router state into the Redux store, so the app has a single source of truth.

I agree it would be nice.

In general, providing lower-level hooks for increased flexibility is very much on the radar.

That said, the priority right now is to 1. release the V1 design and 2. release v1.0.0. As you can imagine it's a lot of work so I need to prioritize accordingly.

While increased control over history.pushState is very much a consideration it isn't a priority. That's why I'm closing this for now; I'll re-open this ticket when it becomes a priority.

In the meantime, upvote this ticket (react with 👍) if you need this. Also feel free to comment here if it's somehow a blocker.

Also, would your company be up for sponsoring? The more we have company sponsors, the faster we can move Vike forward and the faster lower-level hooks are going to see the light. The truth is that I need your help as much as you need mine: it's best when we help each other.

thomasjm commented 11 months ago

Also, would your company be up for sponsoring?

Not at the moment unfortunately, but hopefully someday.

Also feel free to comment here if it's somehow a blocker.

I think I just ran into a blocker situation.

Like I mentioned, I want my app to be able to push new searches to the URL and pick these up in the React UI. The problem arises when you try to use the browser's forward/back buttons.

It seems that Vike's client runtime actually responds to the history popstate event in here. Sometimes Vike decides to try rerendering the page, which in my case is not desirable. I'll list an example sequence of events:

  1. In a new browser tab, open /my/url?search=foo
  2. Press a button in the UI that causes a history.pushState to set search=bar. Now we're at /my/url?search=bar and everything is fine (no rerender happens).
  3. Press the back button: now we're back on /my/url?search=foo. The client router tries to set the scroll position, which is not desirable (this branch). The scroll position change has no effect in my case so everything still looks okay.
  4. Press the forward button: now it tries to go to /my/url?search=bar again. This time we hit this branch, which tries to rerender the page--it redoes my GraphQL queries and screws up the app state.

I think there's an actual design question here:

As an API suggestion, maybe Vike could export a function shouldVikeProcessPopState. By default it always returns true, but the user could override it to look at a given popstate event and return false if they want Vike to ignore it.

thomasjm commented 11 months ago

After posting this I wondered if it might be possible to solve the problem with the onBeforeRender hook, perhaps to abort the render. But it doesn't seem to work.

It would be a convenient API though if I could just define a hook in the page(s) where I want to deal with my own URL search. Maybe the hook could be named onBrowserHistoryEvent.

brillout commented 11 months ago

I think I just ran into a blocker situation.

Correct me if I'm wrong, but I don't think it's a blocker: the workaround is to re-render the whole page while using React's caching mechanisms if you run into performance issues. That's actually the React idiomatic appraoch. (And that's also what Redux does, although an immutable store improves the caching story). I know it isn't what you want, but that's how React works. (Whereas Vue and Solid use an architecture based on signals: everything is essentially always cached while signals surgically update elements.)

That said, I'm open for providing lower-level hooks for increased flexibility, also regarding history.pushState. It just isn't the priority at the moment.

thomasjm commented 11 months ago

Well, I thought about it a while and I think that just letting React re-render does work in principle. But, it's quite inconvenient when you consider the other stuff that often happens in _default.page.client.tsx.

For example, I'm using GraphQL the way the example shows here. If I just let the page re-render, a whole new GraphQL client will be created using the page's original state. So I would have to take extra steps to pull out the current GraphQL client and/or state, and reuse it.

The same goes for preserving my app's Redux state, and every other wrinkle that occurs in a complex app.

I also think that doing a full React re-render is going to be expensive since it has to check the whole tree, though I haven't measured it yet.

Would you potentially accept a PR if I were to start hacking on the onBrowserHistoryNavigation function to make it more configurable?

brillout commented 11 months ago

I'm open for a PR but, before that, we'd need to settle on a feature/design that make sense.

I wonder: your client-side render hook has complete control over what happens on the client-side upon navigation. Correct me if I'm wrong but AFAICT you can actually already do what you want: adapt your render hook accordingly. So instead of interecepting Vike's handling of the history push API, change your client-side render hook.

thomasjm commented 10 months ago

In the last week I did actually figure out a solution that works for me.

The solution was to move the creation of all stateful pieces (like the GraphQL client object, and the Redux store) into a React wrapper component wrapped around the app. They can be created in useState hooks. Then, you can pass the pageContext into this component and update these stateful things as needed with useEffect hooks. The effect of this is that Vike doing a page re-render doesn't disrupt these things.

I'm a little surprised this has worked as well as it has, but my current Frankenstein of Vike with react-router is passing all my tests.

brillout commented 10 months ago

Neat and makes sense. So I guess you're using some kind of cache mechanism (Redux/React?) to avoid a page re-render from triggering the rendering of all the page's components, correct?

Frankenstein of Vike

What makes you think that? Your setup seems legitimate to me. (As I was saying: AFAICT re-rendering the whole page while using a cache is idiomatic React usage.)

thomasjm commented 10 months ago

So I guess you're using some kind of cache mechanism (Redux/React?) to avoid a page re-render from triggering the rendering of all the page's components, correct?

It's just calling React's render function again with the given React nodes, on the same React root (which is defined at module scope so it will be reused). This is a fairly normal thing to do in React and the performance has been acceptable. I believe React does its DOM diffing magic over the entire tree.

What makes you think that? Your setup seems legitimate to me.

Well, it still troubles me that the onBrowserHistoryNavigation function is listening for window state changes, and so is my React router stuff. The code in there is rather complex and I can see that it may have at least one other side effect: calling setScrollPosition on certain hash changes. So I wouldn't want these two things stepping on each other.

I also have to be careful to use history.pushState in some situations and Vike's navigate function in others, which feels a little delicate.