Skip to content

Improvements to history management #2238

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexspeller
Copy link

@alexspeller alexspeller commented Feb 17, 2025

Before I start going deep down the rabbithole making sure this is all cleaned up and tested etc, I'd like to get some feedback on if you're likely to accept this PR.

I've had some issues with inertia's history handling during a migration to inertia with a legacy app.

The problem is that inertia assumes that all history manipulation is handled with inertia's router.push / router.replace methods, which is not the case.

There are two ways this causes issues:

  1. When handling popstate, it assumes that the current history state should be handled by inertia. However it the state was not pushed by inertia, history.state will not contain a valid page object and should be ignored to prevent errors
  2. When preserving scroll positions, the event handler assum
    es that the url stored in currentPage is the correct current url, which it might not be if some other code has pushed a new state onto the stack in the meantime.

This causes a bug that means that if you are currently on and inertia page http://example.com/foo, and run pushState({}, "", "http://example.com/foo?bar=123"), you end up in a situation that inertia can't handle well
a. If you scroll the page, the scroll handler will pushState with the latest inertia page url it knows about, and the query param will disappear
b. If you hit the back button, inertia will try to handle the pop state event with the empty state with no inertia page in it, and will error out trying to render a component.

I would like to fix these issues to make inertia more interoperable especially during stack migrations, so please let me know if going down this path is likely to align with the project goals and if so I'll improve the handling here to make things a little smoother

Note: I'm not actually sure just an option to turn off scroll preservation makes the most sense here - it might make more sense to make the scroll preservation smarter such that it handles non-inertia history state more gracefully but still works with inertia state

@mochetts
Copy link
Contributor

I had the same issue recently. 👍🏻

@pascalbaljet
Copy link
Member

Could you give me an example of how/why you are doing manual history manipulation in an Inertia app? In v2 you can use Client side visits to change just the URL:

import { router } from '@inertiajs/vue3';

router.replace({
    url: '/?foo=bar',
    preserveScroll: true,
    preserveState: true,
});

@alexspeller
Copy link
Author

I am migrating a large app slowly to inertia. It has lots of uses of pushState and replaceState already in the app. I am currently using a forked version of inertia with these changes to support this.

The client side api is great, but it doesn't help with legacy code, or 3rd party code over which we don't have control - it's fine but it would be better to not break history.pushState / history.replaceState browser apis when it's not necessary to do so. It limits the compatibility of inertia with other ecosystem components and legacy apps when migrating.

@joelstein
Copy link

@pascalbaljet Here's a use case I wrote about last year:

We have several big data pages that include all data on the initial page load and are filtered through various form controls. These settings are mirrored to the URL so users can bookmark the link and get the same options.

@pascalbaljet
Copy link
Member

I'm still not fully sure if we should support this. Up until now, we assume that Inertia is the only package that handles the History state. If we open this up, this might lead to a ton of issues with incompatible third-party packages. For legacy code, I'd strongly suggest migrating to the official way of doing this with the Client Side Visits feature that was introduced in v2.

But taking a technical look at this PR, could you explain how this problem was solved?

When handling popstate, it assumes that the current history state should be handled by inertia. However it the state was not pushed by inertia, history.state will not contain a valid page object and should be ignored to prevent errors

If I understand the new code correctly, the scroll handling becomes optional. This would be a breaking change. How would you enable it again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info/work Needs more info from the author or additional work to get merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants