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

[Discuss] Browser location management in NP #58217

Closed
Dosant opened this issue Feb 21, 2020 · 4 comments
Closed

[Discuss] Browser location management in NP #58217

Dosant opened this issue Feb 21, 2020 · 4 comments
Labels
discuss Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@Dosant
Copy link
Contributor

Dosant commented Feb 21, 2020

Browser location management in NP

Working on state syncing utils and AppState / GlobalState replacement #44151 made me thinking that NP is probably missing some primitives (or documented guidance) for working with browser location.

I think we didn't bump into many blocking issues so far because of the transitional
period where most apps are using angular, hashed routing and navigation between them could happen with full browser page reload.

But as we start to see more apps cut-off in new platform, some issues started to come up #58112, #53692

I tried to outline some other possible issues which would likely become important on some point.
Especially as more apps are migrated to NP, stop using angular and hashed routing. And when kibana is a compete single page application.

Assuming platform is still handling mounting and unmounting apps using react-router and history I can think of following problems:

  1. Deep-linking between NP apps
  2. Listening for url changes
  3. Batching url updates
  4. Scroll restoration ?

Deep linking between NP apps

Problem

Core provides an imperative api for navigation between apps.

navigateToApp(appId: string, options?: {
        path?: string;
        state?: any;
    }): Promise<void>;

But apps and plugins would also create regular html links to other places in kibana

<a href="/dashboard/${id}/?_a=${state}"></a>

With current NP setup clicking on such link would likely not properly work,
because platform's react-router won't be notified about this location change.

To make this link work properly it have to be rendered with react-router's <Link/>. App's <Router/>
instance have to use ScopedHistory provided by core.

But if all the code that renders links have to use <Link/> it means it have to also use React which is not framework-agnostic.

Why this wasn't a problem before?

There is a window event hashchange. But it doesn't work for not hash urls. So this becomes a problem when we move to single page app setup with urls without #.

Possible solution

  1. Agree and document that using <Link/> wrapped in core's ScopedHistory instance is crucial. (not likely as it enforces to use react any rendering)
  2. Add global event handler in core which captures all a[href] clicks and notifies it's react-router about navigation between apps. Something similar to this but for all links.

Listening for url changes

Problem

Before NP, apps or plugins could listen for url change using angular's $location or hashchange event.
In NP ScopedHistory.listen should be sufficient for apps (assuming all the url updates are happening using correct history instance).

But we recently bumped into the case, where we wanted to explore managing part of the url from the data plugin. cc @lizozom @timroes (even though this idea diverges from #39855)
We thought of making plugin subscribe for url changes in the setup phase. Plugin would also have to be able to do query updates.

Possible solution

  1. Core could expose it's global history instance? Or expose just listen callback and a centralised way to update query params so all listeners would be properly notified?

Batching url updates

Problem

In NP we don't have a mechanism to batch multiple url changes into one browser history stack update.
Before NP we could use $location. It batched all url changes and applied them in next digest cycle.

To replace AppState and GlobalState which relied on this $location's behaviour, we implemented query updates batching inside state_sync utils. more.
But only state_sync benefits from this right now.

The usage ergonomics of these utils is also not good, as developers should always use the same history / kbnUrlStateStorage for correct simultaneous state syncing of different sources with url. Because of this we can't setup state syncing with url from two different places as kbnUrlStateStorage, which does the batching, is not available globally. As an example: we want separately setup syncing of data plugin state with _g query param + and application state with _a query param)

Solution

  1. An api (maybe extension of history) which would allow to batch url updates.
  2. For specific AppState / GlobalState replacement use case kbnUrlStateStorage solves this, We could continue to implement local solutions for this.

Scroll restoration

@Dosant: todo, investigate if this is a problem and what is the impact.

@Dosant Dosant added discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform Team:AppArch labels Feb 21, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@joshdover
Copy link
Contributor

  1. Agree and document that using <Link/> wrapped in core's ScopedHistory instance is crucial. (not likely as it enforces to use react any rendering)
  2. Add global event handler in core which captures all a[href] clicks and notifies it's react-router about navigation between apps. Something similar to this but for all links.

I don't think we have to add a global event handler in order to still maintain our framework-agnostic goals of Core. We could recommend using <Link> while also documenting an example of how to do this with vanilla JavaScript.

That said, I'm not necessarily against a global event handler if it greatly simplifies this for plugins (which I suspect it might). I think it's something worth exploring and making sure there aren't any edge cases that could be hard to handle.

Core could expose it's global history instance? Or expose just listen callback and a centralised way to update query params so all listeners would be properly notified?

I do not really like this option. I think much of the confusion in our legacy URL management came from global mechanisms changing the routing behaviors for all applications. However, if just listen is needed I would be comfortable exposing that without exposing any of the writable History methods.

What is the exact use case here that the opt-in approach outlined in #39855 does not work for?

An api (maybe extension of history) which would allow to batch url updates.

I think this makes sense to possibly bake into ScopedHistory. However, I think we should make sure this is not the default behavior but can be specified with an option to the push method.

@joshdover
Copy link
Contributor

I've split this issue into 3 separate issues so each can be prioritized accordingly by the platform team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

3 participants