elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.65k stars 8.23k forks source link

Document workaround for using HashRouter + recommendation for migrating to HTML5 routing #65968

Closed pgayvallet closed 4 years ago

pgayvallet commented 4 years ago

In the discussion below, we decided to simply document a workaround and recommendation for moving away from hash routing.

Original Issue

In https://github.com/elastic/kibana/pull/63443, lots of legacy apps were adapted to use NP apis, but most of these apps are still using a hash-based history/navigation.

However, as

pushState() never causes a hashchange event to be fired, even if the new URL differs from the old URL only in its hash.

Navigation using push from the app's scoped history or from the global applications history is 'missed' from the hash history used in the apps. Most notable usages are using the navlink of the current active app, or using application.navigateToApp to navigate to the active app.

Mid-term, all apps should move away from hash history and use our ScopedHistory instead, however this should not be blocking migration, so we need to find a solution for the transition period.

Also one could argue that using 'html5'/'pushState' or 'hash' history should be an implementation detail / a decision that belongs to the plugin developers and that core should just expose a way for plugins to properly uses a hash history if they want to.

After discussion, the suggested approach would be to add an additional API to our ScopedHistory implementation to allow to create a 'scoped' hash history.

    core.application.register({
      id: 'foo',
      title: 'foo',
      mount: ({ history }: AppMountParameters) => {
        const hashHistory = history.createHashHistory();
         // create a <Router history={hashHistory} .../>
        return () => undefined;
      }
    })

This would be strongly based on the base hash history implementation (https://github.com/ReactTraining/history/blob/master/modules/createHashHistory.js)

Biggest technical challenge would be to decide exactly how we hook the scoped history with it's hash counterpart (in both ways).

We will probably need to create synthetic hashchange events when it's needed. This work around is what is (manually) performed in https://github.com/elastic/kibana/pull/63443

cc @joshdover @flash1293

elasticmachine commented 4 years ago

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

joshdover commented 4 years ago

Biggest technical challenge would be to decide exactly how we hook the scoped history with it's hash counterpart (in both ways).

We will probably need to create synthetic hashchange events when it's needed. This work around is what is (manually) performed in #63443

My biggest concern with this proposal is that getting this 100% right may actually turn out to be quite difficult. If that turns out to the be case, then we need to decide whether or not supporting this is actually something we want to do.

I also think about the cognitive load of support two routing solutions isn't really something we may want to deal with long-term. Ideally, we have one, flexible way to do routing in Kibana.

Given that, the only reason I can see to implement this would be to provide a temporary workaround while teams work on migrating away from hash-based routing. Would providing this utility (ideally, outside of Core's API) save plugin teams time and pain and can we provide it in a reasonable timeframe? If not, it's probably worth having teams work on migrating to HTML5 routing now instead of later.

pgayvallet commented 4 years ago

My biggest concern with this proposal is that getting this 100% right may actually turn out to be quite difficult

I definitely agree

Ideally, we have one, flexible way to do routing in Kibana.

hashbang routing is usually proposed as an alternative to html5 routing in frameworks, as using hashbang routes is always a little easier than html5 routes, that requires the server to be able to serve the page from any 'virtual' route.

As the Kibana server is already properly working with html5 routes, this argument in not valid in our case, and I don't really see another good reason to use hash history except for 'legacy' routing support.

Would providing this utility (ideally, outside of Core's API) save plugin teams time and pain and can we provide it in a reasonable timeframe? If not, it's probably worth having teams work on migrating to HTML5 routing now instead of later.

Ideally, we would just document that hash history / router should be dropped in favor of browser history when migrating apps to KP, and that KP is not officially supporting hash history.

In that case, we could just provide an event-based workaround, or at least documentation on the possible workaround, which would be way easier than a custom HashHistory implementation.

@flash1293 If I remember correctly, when migrating the legacy 'kibana' apps, your workaround was just to manually emit hashchange events when the root history changed, right? I think this would be an acceptable workaround for an other app still using hash history?

flash1293 commented 4 years ago

So far we didn't experience any fundamental problems with this, and most problems are probably inherent to hash routing (https://github.com/elastic/kibana/issues/65090). I'm fine with your approach, if tons of problems pop up in the future we can still reconsider.

The downside of a non-centralized solution: The things breaking if that workaround isn't applied are subtle in most cases (e.g. clicking the navlink in the sidebar doesn't take you back to the start page of an app if the app is already opened). This is currently the case in ML (also using hash routing) because this workaround is missing there. It might be a problem in a bunch of other places as well, I haven't checked.

flash1293 commented 4 years ago

@joshdover I created an issue here for the Kibana App team: https://github.com/elastic/kibana/issues/67470

But it seems like there are a lot of other teams also affected by this (didn't do a thorough check, but it seems like all apps are currently using hash routing). We should probably make this more widely known.

joshdover commented 4 years ago

Ideally, we would just document that hash history / router should be dropped in favor of browser history when migrating apps to KP, and that KP is not officially supporting hash history.

In that case, we could just provide an event-based workaround, or at least documentation on the possible workaround, which would be way easier than a custom HashHistory implementation.

This path forward makes sense to me. Let's add a section to the MIGRATION_EXAMPLES doc on how to workaround this and then I can email all teams still using hash routing on how to use this workaround and call out that only HTML5 routing is officially supported moving forward.

pgayvallet commented 4 years ago

@joshdover Any idea where we should put this documentation and example of workaround ? Is MIGRATION_EXAMPLES the correct place?

joshdover commented 4 years ago

@joshdover Any idea where we should put this documentation and example of workaround ? Is MIGRATION_EXAMPLES the correct place?

I think that's the best place we have for now.