[Bug] Possible bugs due to different instances of history in NP apps

[Dosant]

Summary

While digging into new state management utils #53582 I bumped into following issue in an example np app I created. The app uses react-router.

The bug is: App's react-router doesn't trigger re-render when clicking on current app link in chrome's side nav.

Reproduction

Video: https://drive.google.com/open?id=17zj0hPR96lAeRn_ehJEMFK3kh-LkbFkP
To run the same and to see code it's: #53582

Cause

It looks like it happening, because np app navigation is happening using core's instance of history https://github.com/elastic/kibana/blob/master/src/core/public/chrome/ui/header/header.tsx#L325, and app react-router's history instance is different one, so listen cb doesn't fire for events fired by core's history and react-router can't trigger rerender.

Possible solution

1. Expose core's history instance for apps to pass into react-router?
   This will cause complications with base path... as it is different for core and apps. Apps will have to append {appBasePath} to react-router configs ... so this is likely not an option
2. If that use case I found is the only one: If that use case I found is the only one, then we could add additional if(appIsActive){ ... } branch here: https://github.com/elastic/kibana/blob/master/src/core/public/chrome/ui/header/header.tsx#L325and notify current app that user clicked on it's link and let app handle it?
   Or, just reload the page.

?

[elasticmachine]

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

[elasticmachine]

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

[joshdover]

One possible option, similar to (1), would be for Core to expose a history-compatible wrapper around Core's history instance that handles the base path translation for the currently mounted application.

Here's the API surface for History:

export interface History<HistoryLocationState = LocationState> {
  length: number;
  action: Action;
  location: Location<HistoryLocationState>;
  push(path: Path, state?: HistoryLocationState): void;
  push(location: LocationDescriptorObject<HistoryLocationState>): void;
  replace(path: Path, state?: HistoryLocationState): void;
  replace(location: LocationDescriptorObject<HistoryLocationState>): void;
  go(n: number): void;
  goBack(): void;
  goForward(): void;
  block(
    prompt?: boolean | string | TransitionPromptHook<HistoryLocationState>,
  ): UnregisterCallback;
  listen(listener: LocationListener<HistoryLocationState>): UnregisterCallback;
  createHref(location: LocationDescriptorObject<HistoryLocationState>): Href;
}

On first glance, it appears only these methods would need to be wrapped to handle basePath issues:

• location
• push
• replace
• listen
• createHref

We would also want to consider wrapping length, go, and goBack.

This seems like a feasible option that doesn't have any drawbacks (other than implementing/maintaining this). @Dosant thoughts?

[Dosant]

One possible option, similar to (1), would be for Core to expose a history-compatible wrapper around Core's history instance that handles the base path translation for the currently mounted application.
This seems like a feasible option that doesn't have any drawbacks (other than implementing/maintaining this)

Seems good to me, if this history have to be used only for navigating within an app, right?
But wondering if we should consider a case, when app wants to navigate to other app?
e.g. I am in dashboard and want to be able to go to visualise with history.push('/app/visualise/')
Or do we have other mechanism for that in place?

Looking at my thoughts in the original description, I am now questioning if having this necessity for apps to "not forget" about base path is actually that bad.

Just thinking:
What if we pivot and consider having 1 instance of history without any patches and enable app developers to append base path of their apps as they need it? Also if needed, we could help them with simple pathname (string) and location (Location) utilities in kibana_utils for adding/removing base path.

Also I see, that in next version of 'history', there will be a convenient singleton to use: https://github.com/ReactTraining/history/releases/tag/v5.0.0-beta.2

import history from 'history/browser';
// And away you go!

[joshdover]

But wondering if we should consider a case, when app wants to navigate to other app?
Or do we have other mechanism for that in place?

We do, there is a core.applications.navigateToApp API.

What if we pivot and consider having 1 instance of history without any patches and enable app developers to append base path of their apps as they need it?

Since Core dictates the appBasePath, I really would like applications to not have to worry about this at all. That way we can easily change the schema we use for applications paths in the future and we don't have to have basePath code littered throughout the code base.

I'm also not sure it's a good idea for applications to have access to the full navigation history. It seems that an app should only be able to see the history stack that has happened while that app has been mounted. I worry about tight coupling that could occur through the history if the full history stack is exposed.

[pgayvallet]

Apps will have to append {appBasePath} to react-router configs ... so this is likely not an option

One possible option, similar to (1), would be for Core to expose a history-compatible wrapper around Core's history instance that handles the base path translation for the currently mounted application.

Can't core just provide a wrapped <Route> component that would prepend the basepath ? It seems way less complicated that maintaining a wrapped History at first glance?

[joshdover]

Can't core just provide a wrapped <Route> component that would prepend the basepath ? It seems way less complicated that maintaining a wrapped History at first glance?

I believe that would actually be more work because <Route> exposes history, location, and match to components that it renders. But since <BrowserRouter> accepts a history object on it's own, I believe we can get away with just wrapping that object and have everything else flow from there.