[Infra UI] Use alternative query string serialization function

[weltenwort]

This replaces the standard queryString.encode/decode functions with Kibana's specialized querystring serialization functions which seem to work around angularjs $router quirks.

As a result, most local state changes in the Infrastructure UI no longer lead to an ever-expanding list sequence of history states. This restores the forward- and backward-button navigation between sites of the app.

fixes #24028

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[skh]

For me this doesn't implement the desired behavior as described in #24028 . Instead, the back button jumps back to /app/kibana#/home?_g=(). Is this intentional?

[Kerry350]

For me this doesn't implement the desired behavior as described in #24028 . Instead, the back button jumps back to /app/kibana#/home?_g=(). Is this intentional?

I also see the same behaviour.

[weltenwort]

True, it doesn't represent every small state change in the history. It merely fixes the bug that navigating "one step back" via the browser chrome or the history API doesn't have any effect.

The code was written to track navigation between apps or sections of apps (i.e. other kibana apps, the waffle map, the node details pages and the logs). I'd be interested to hear what the arguments for a different navigation paradigm would be.

[weltenwort]

Maybe we should narrow the original bug report to the "navigating back does not have any effect" aspect and split off a separate "what state changes should be represented in the browser history" discussion?

[Kerry350]

As this bug fix is aimed at path changes causing history entries, and not query parameters, only I'm happy to LGTM 👍 this.

Deferring discussion from the meeting:

Do we want path changes only to create new history entries (pushState) and query parameters reflected with replaceState, which will provide back button functionality to path changes, but not query parameters? (Current behaviour)

Or

Do we want path and query parameter changes to create pushState history entries, resulting in both responding to the back button? (Potential behaviour)

Originally with the review I was (as I think Sonja was) basing my comment off of the comment in #24028 and testing that as the expected behaviour.

As for what I'd deem expected behaviour, I think what we have now is good.

If there's one thing we don't want to do - in my previous experience - is to try and mix and match the two (i.e. sometimes query parameters cause a new history entry, sometimes they don't). We currently have path for persistent parameters, navigable with back, and query parameters for rapidly changing filters, sorts etc.

The ultimate goal is that a URL can be used to 100% replicate what a user was looking at, and that it can be shared with others, that we have.

There is one area where I'm torn though, and that's pagination (in general, not even just aimed at our infra codebase). Pagination parameters end up in query parameters generally, but I would expect to be able to use the back and forward buttons to navigate between page changes.

[weltenwort]

Good points, @Kerry350, thank you. So maybe we actually do want to mix-and-match for some cases like pagination (to be decided once that use cases arises). Just for reference, the pagination state of most tables in Kibana is not represented in the URL.

[Kerry350]

So maybe we actually do want to mix-and-match for some cases like pagination (to be decided once that use cases arises).

Yeah — to be honest we probably do. "If there's one thing we don't want to do" was a bit hasty, the previous experience I speak of was manually adding query parameter route change support into an old version of the Backbone library router, and then getting that to synchronise nicely with duplicated Redux state. It was a nightmare, but it is not reflective of the landscape these days 😄

Just for reference, the pagination state of most tables in Kibana is not represented in the URL.

👍 Cool, another thing to bear in mind then, best to keep consistency throughout the app.

Ultimately, I think if we did want to add support for the query parameters, and also make it so it had the power to be mix and match, if we sat down and planned out an API as a team we could come up with something really nice (only if the need ever arises).

Also, two extra things:

• Nice one fixing this, I know it's caused quite some headaches for a while!

• I would second what Chris said in the meeting in that as a whole the URL state container works much nicer than things I've worked with in the past. I haven't used it properly yet, but I had a good look at it the other day.

[makwarth]

Thanks for the write up @Kerry350. I agree with your comment, so this PR LGTM as well.
I do think it's counter-intuitive that navigating the primary navigation (the node types) doesn't trigger path changes, but that's a problem with the node type navigation (it shouldn't be primary), and not the pushState logic. (Related: Issue for changing the primary navigation: #27916)

[weltenwort]

Thanks for the input, everyone!

another thing to bear in mind then, best to keep consistency throughout the app

That's a good general guideline, but there's lots of unfortunate, historically grown behaviour in Kibana that we would probably do well to ignore. 🕸️ In this case maybe the lesson from Kibana would be to use the URL sparingly for state that really needs to be bookmarkable/sharable.

[makwarth]

Great job getting this fixed @weltenwort !