Kibana app migration: Local application service #48898

flash1293 · 2019-10-22T12:55:36Z

Introducing a local application service simulating the core application service within the kibana app LP plugin to make routing without reload possible.

The individual apps register themselves to the local application service and can do their own routing within #/${appId}/... - the local application service just listens for changes in the first path segment and unmounts/mounts the correct app. For now this is realized with the global angular router but once everything in the kibana app plugin is using the local application service, it can be retired in favor of something super lightweight like a window.onhashchange listener. While the global angular routing is used, it is important to make sure that triggered listeners won't interfere with the loaded app (e.g. resetting badges or help texts). Because of this the handlers look for a special flag marking the "wrapper routes" and exit early if they are encountered.

As it is using the global angular router for now, it is possible to do a step-wise migration, e.g. moving the home app to a pure react app while the other parts of the Kibana plugin are still using the global angular router (#48715).

Because discover will need this for the shimming, it also introduces a forward method that can be used to forward all URLs matching a certain prefix to another prefix. For now this feature will only migrate URLs within the kibana app route (everything after the hash), but when the individual plugins are actually moved to the new platform, the local application service will be the only thing left in the kibana plugin making sure all old URLs are forwarded correctly to their NP counterparts.

When moving the responsibility for the routing into the individual sub-apps, it's important to make sure unknown urls are redirected to the correct place because the catch-all redirect on kibana plugin level doesn't trigger anymore. The flow works like this:

Global angular bootstraps
Kibana LP plugin starts
Local application service owns the routing - it checks the first path segment to determine which app to mount
Local application service mounts the corresponding app
The app bootstraps (might use angular, might use react, might use vue) and takes over ownership over the routing. Local application service just listens in to check whether it has to unmount the current app and mount another app.
When switching between routes within the current prefix, the app does it's own thing
When switching to a URL within the current prefix that doesn't has a real route (e.g. #/discover/this/is/not/a/real/path, the app router should redirect to #/{chrome.getInjected('kbnDefaultAppId')} (because this is the current behavior, if you go to an unknown route within let's say visualize, you get redirected to home - we can discuss to change this and have an app-specific behavior which might make more sense)
When switching to a URL outside of the current prefix, the app router shouldn't do anything because it doesn't own the routing anymore - the local application service takes over routing again, unmounts the current app and mounts the next app.

In case of react router the config might look like this:

const myAppId = 'myApp';
return (<Router>
  <Switch>
    {/* Register paths of the app itself*/}
    <Route exact path={`/${myAppId}/path1`} render={renderPath1} />
    <Route exact path={`/${myAppId}/path2`} render={renderPath2} />
    <Route exact path={`/${myAppId}/path3`} render={renderPath3} />
    {/* Everything else that starts with the app id (notice the missing exact flag) gets redirected */}
    <Route path={`/${myAppId}`}>
      <Redirect to={`/${chrome.getInjected('kbnDefaultAppId')}`} />
    </Route>
    {/* Do NOT use <otherwise />, because it will also redirect navigations to other apps that should be left to the local application service to handle */}
  </Switch>
</Router>);

In case of local angular in might look like this:

const myAppId = 'myApp';
// Register paths of the app itself
routes.when(`${myAppId}/path1`, { template: path1Template });
routes.when(`${myAppId}/path2`, { template: path2Template });
routes.when(`${myAppId}/path3`, { template: path3Template });
// Everything else that starts with the app id gets redirected (`:tail*` is a placeholder variable that spans segments, so it matches everything that starts with the app id but isn't caught be the actual routes)
routes.when(`${myAppId}/:tail*?`, { redirectTo: `/${chrome.getInjected('kbnDefaultAppId')}` });

// Do NOT use routes.otherwise, because it will also redirect navigations to other apps that should be left to the local application service to handle

This PR can be tested by adding a small demo app, registering it in the local application service and check whether handles are called in the correct places.

elasticmachine · 2019-10-22T14:01:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1bd6b3b

elasticmachine · 2019-10-24T16:26:48Z

Pinging @elastic/kibana-app (Team:KibanaApp)

flash1293 · 2019-10-24T16:26:56Z

@elasticmachine merge upstream

elasticmachine · 2019-10-24T17:51:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 48ecdf7

kertal · 2019-10-28T10:39:43Z

src/legacy/core_plugins/kibana/public/local_application_service/local_application_service.ts

+
+    this.forwards.forEach(({ legacyAppId, newAppId, keepPrefix }) => {
+      angularRouteManager.when(matchAllWithPrefix(legacyAppId), {
+        redirectTo: (_params: unknown, path: string, search: string) => {


A quick note while testing, when search is an object, this produces links like:
http://localhost:5601/tmz/app/kibana#/discover/doc/indexPattern/index?%5Bobject%20Object%5D&_g=()

Ah, good catch, thanks. I will look into this.

OK, this should be fixed by using a resolveRedirectTo constructing the redirect by using $location.url.

…pplication-service

…h1293/kibana into flash1293/local-application-service

elasticmachine · 2019-10-28T11:57:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 79a1a14

src/legacy/core_plugins/kibana/public/local_application_service/local_application_service.ts

src/legacy/ui/public/legacy_compat/angular_config.tsx

timroes · 2019-10-28T17:13:44Z

src/legacy/core_plugins/kibana/public/local_application_service/local_application_service.ts

+}
+
+const matchAllWithPrefix = (prefixOrApp: string | App) =>
+  `/${typeof prefixOrApp === 'string' ? prefixOrApp : prefixOrApp.id}:tail*?`;


Shouldn't there be a / before the :tail*??

timroes · 2019-10-28T17:19:28Z

src/legacy/core_plugins/kibana/public/local_application_service/local_application_service.ts

+        controller($scope: IScope) {
+          const element = document.getElementById(wrapperElementId)!;
+          (async () => {
+            const onUnmount = await app.mount({ core: npStart.core }, { element, appBasePath: '' });


This function looks like it would break if we're navigating away while we're still mounting the app, since we're attaching a listener on a then destroyed scope? We could potentially use the private scope.$$destroyed to check if it actually is already destroyed once app mount has finished?

I believe the appBasePath here shouldn't be empty. It probably should be the id of the current legacy app + the id of the app being mounted. For example, if I registered a app with id dashboard, I'd expect the appBasePath to be something like: chrome.basePath.prepend('/kibana#dashboard')

Is this really helpful? AFAI understand the appBasePath is passed in so the application can make sure it's prefixing all links and routes correctly, right? So the contract is basically "if something changes after this prefix, it's up to you to handle it". If the base path switches technologies going from a path to a hash, this will require really weird code within the app to handle both cases (checking whether there is a hash in there and switching from history router to hash router or something like that).

I'm also not sure whether we can provide a meaningful "base path" here yet. Currently all consumers of the local application service have to use hash routing and make sure they handle the app/kibana part as well as the #/dashboard part of the url, so we have to do changes to the routing in this regard anyway when switching to the new platform and the real application router. The compat layer for legacy platform seems to follow the same principle: src/legacy/ui/public/new_platform/new_platform.ts (it also has the same problem Tim is mentioning above).

@joshdover I'm going to merge this PR because it's blocking several other PRs to become reviewable and it's not actively used anywhere for now. If you still have concerns we can continue the discussion and adjust things in a separate PR.

elasticmachine · 2019-10-28T20:00:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5195ebf

timroes · 2019-10-29T13:20:42Z

src/legacy/core_plugins/kibana/public/local_application_service/local_application_service.ts

-        redirectTo: (_params: unknown, path: string, search: string) => {
-          const newPath = `/${newAppId}${keepPrefix ? path : path.replace(legacyAppId, '')}`;
-          return `${newPath}?${search}`;
+        resolveRedirectTo: ($location: any) => {


ng.ILocationService

timroes · 2019-10-29T13:21:02Z

src/legacy/core_plugins/kibana/public/kibana.js

 import { localApplicationService } from './local_application_service';

-localApplicationService.apply(routes);
+localApplicationService.forwardApp('foo', 'discover');


Don't forget to remove that debug forward before merging :-)

…pplication-service

elasticmachine · 2019-10-30T08:53:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a724d7b

kertal

Code LGTM, tested forwarding in my Discover shim with doc and context, works 👍

local application service

1bd6b3b

flash1293 added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.5.0 Feature:NP Migration labels Oct 22, 2019

flash1293 marked this pull request as ready for review October 22, 2019 14:05

flash1293 requested review from joshdover, kertal and timroes October 22, 2019 14:05

flash1293 added the Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// label Oct 24, 2019

Merge branch 'master' into flash1293/local-application-service

48ecdf7

kertal reviewed Oct 28, 2019

View reviewed changes

flash1293 added 2 commits October 28, 2019 11:41

Merge remote-tracking branch 'upstream/master' into flash1293/local-a…

74fb184

…pplication-service

Merge branch 'flash1293/local-application-service' of github.com:flas…

79a1a14

…h1293/kibana into flash1293/local-application-service

timroes reviewed Oct 28, 2019

View reviewed changes

review fixes

5195ebf

timroes reviewed Oct 29, 2019

View reviewed changes

flash1293 added 2 commits October 30, 2019 08:34

Merge remote-tracking branch 'upstream/master' into flash1293/local-a…

6bc61a8

…pplication-service

review fixes

a724d7b

flash1293 mentioned this pull request Oct 30, 2019

Shim home app #48715

Merged

kertal self-requested a review October 30, 2019 10:26

kertal approved these changes Oct 30, 2019

View reviewed changes

flash1293 merged commit 1fab577 into elastic:master Oct 30, 2019

flash1293 deleted the flash1293/local-application-service branch October 30, 2019 11:15

flash1293 mentioned this pull request Oct 30, 2019

[7.x] Kibana app migration: Local application service (#48898) #49724

Merged

flash1293 added a commit to flash1293/kibana that referenced this pull request Oct 30, 2019

Kibana app migration: Local application service (elastic#48898)

bca71d3

flash1293 added a commit that referenced this pull request Oct 30, 2019

Kibana app migration: Local application service (#48898) (#49724)

28314ba

Kibana app migration: Local application service #48898

Kibana app migration: Local application service #48898

Uh oh!

Conversation

flash1293 commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Oct 22, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Oct 24, 2019

Uh oh!

flash1293 commented Oct 24, 2019

Uh oh!

elasticmachine commented Oct 24, 2019

💚 Build Succeeded

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 28, 2019

💚 Build Succeeded

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 28, 2019

💚 Build Succeeded

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 30, 2019

💚 Build Succeeded

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flash1293 commented Oct 22, 2019 •

edited

Loading

flash1293 Oct 28, 2019 •

edited

Loading