Fix remember with externally updated querystring

[AdrianMrn]

In our inertia-react application, we're using Axios to fetch new data when a filter in an index page changes. We also then update the querystring, so the URL to the filtered index page is shareable. We're storing all data using useRememberedState(), so when navigating to a detail page and pressing the back button, the index page hasn't changed from how it looked before (this works terrifically btw, including saving the scroll position in an infinite scroll container and everything).

After upgrading inertia from 0.4.1 to 0.6.x, the querystring is now updated by our application, only to revert to what it was before a second later. After some sleuthing, it looks like this update to the remember() function in [email protected] is the culprit:

https://github.com/inertiajs/inertia/compare/v0.4.1...v0.4.2#diff-467a987931305e1cb6da2436149dad1d780e881e4df994731a88c53bbdf53b08R347-R354

The remember function is called by the useEffect hook in useRememberedState in inertia-react when I update my data: https://github.com/inertiajs/inertia/blob/master/packages/inertia-react/src/useRememberedState.js#L11-L13

At this point, this.page does not yet include my change to the querystring, so my change gets overwritten.

I've changed it back to work like it did before, getting the state from window.history.state instead of the (untouchable by external changes) this.page. This seems to put everything back in working order, and remembered state still works properly on navigating through the state.

Please let me know if I overlooked anything with this change that could break another part of inertia.

[reinink]

Hey there @AdrianMrn! Thanks for contributing this, and for your excellent explanation of the issue.

As you can probably tell from my silence on this, I'm kind of torn on what to do here.

On one hand, this feels like a reasonable enough change, although I do question why I switched it from the way it previously was. That was quite a while ago, and I can't remember if that was to correct a bug, or just a coincidental change.

On the other hand, I'm not really sure if this is an approach we want to encourage with Inertia. The goal with Inertia is to have Inertia manage your client-side history state. Modifying history yourself via pushState or replaceState is likely to cause other issues as well, as Inertia will become out of sync with what the current history state is.

Generally in these situations I recommend letting Inertia manage this history state by making regular Inertia visits, instead of using axios and updating the query strings yourself. Is there something in particular stopping you from using Inertia for this instead of axios? You can see an example of how we do this in Ping CRM (see here and here).

I am going to leave this PR open for now, because I want to do more testing with this, but for anyone else viewing this, my recommendation would be to not manually edit history state yourself. 👍

[reinink]

Note to self: explore removing the page object entirely from Inertia core, and just relying on the history state object directly.

[mattkingshott]

I feel like this is a valid use-case as I mentioned in my other issue. Not everything requires a visit IMO, sometimes navigating within the same page, such as with a tab component.

Maybe the solution is a way to hook into the replaceState so we can add extra stuff or update the query string (kinda similar to the laravel middleware).

Just some thoughts.

[reinink]

Yeah, not saying it's not a valid use-case, it's just not as simple as it sounds. Inertia operates on the premise that all routes are manage server-side. Introducing client-side only routing, even in a simple form, is non-trivial, since it breaks from the paradigm that Inertia operates in. Meaning, just because a request isn't necessary, it might still be the best way to do this given how Inertia is currently designed.

That said, it's something I have considered in the past, and will certainly keep it in mind for the future.

[mattkingshott]

That makes sense.

I guess all I'm saying is that it would be nice to be able to "route" around the current page, not anything that would necessitate moving off-page, which I would imagine would be easier, albeit far from simple.

That said, the localVisit idea sounds like a good approach, though I would humbly suggest that enabling replaceState as well as pushState would be quite useful. The reason being that navigating around tabs on the same page doesn't necessitate adding a history item (IMO).

[mattkingshott]

As an update, I switched away from my mini client side routing and now just use Inertia. While it required some hefty refactoring on my end, I do feel it was the right approach.

Thanks @reinink 👍🏻

[reinink]

Hey! Thanks so much for your interest in Inertia.js and for submitting this contribution.

In an attempt to get on top of the issues and pull requests on this project I am going through all the older issues and PRs and closing them, as there's a decent chance that they have since been resolved or are simply not relevant any longer. My hope is that with a "clean slate" me and the other project maintainers will be able to better keep on top of issues and PRs moving forward.

Of course there's a chance that this PR is still relevant, and if that's the case feel free to re-submit it. If it's a new feature and not a bug fix maybe respond here first to make sure that it's something we want to include in the library.

Really not trying to be dismissive here, I just need to find a way to get this project back into a state that I am able to maintain it. Hope that makes sense! ❤️