Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/legacy/core_plugins/kibana/public/kibana.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ import 'ui/agg_response';
import 'ui/agg_types';
import { showAppRedirectNotification } from 'ui/notify';
import 'leaflet';
import { localApplicationService } from './local_application_service';

localApplicationService.apply(routes);

routes.enable();

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export * from './local_application_service';
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { App } from 'kibana/public';
import { UIRoutes } from 'ui/routes';
import { IScope } from 'angular';
import { npStart } from 'ui/new_platform';
import { htmlIdGenerator } from '@elastic/eui';

interface ForwardDefinition {
legacyAppId: string;
newAppId: string;
keepPrefix: boolean;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* To be able to migrate and shim parts of the Kibana app plugin
* while still running some parts of it in the legacy world, this
* service emulates the core application service while using the global
* angular router to switch between apps without page reload.
*
* The id of the apps is used as prefix of the route - when switching between
* to apps, the current application is unmounted.
*
* This service becomes unnecessary once the platform provides a central
* router that handles switching between applications without page reload.
*/
export class LocalApplicationService {
private apps: App[] = [];
private forwards: ForwardDefinition[] = [];
private idGenerator = htmlIdGenerator('kibanaAppLocalApp');

/**
* Register an app to be managed by the application service.
* This method works exactly as `core.application.register`.
*
* When an app is mounted, it is responsible for routing. The app
* won't be mounted again if the route changes within the prefix
* of the app (its id). It is fine to use whatever means for handling
* routing within the app.
*
* 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.
*
* @param app The app descriptor
*/
register(app: App) {
this.apps.push(app);
}

/**
* Forwards every URL starting with `legacyAppId` to the same URL starting
* with `newAppId` - e.g. `/legacy/my/legacy/path?q=123` gets forwarded to
* `/newApp/my/legacy/path?q=123`.
*
* When setting the `keepPrefix` option, the new app id is simply prepended.
* The example above would become `/newApp/legacy/my/legacy/path?q=123`.
*
* This method can be used to provide backwards compatibility for URLs when
* renaming or nesting plugins. For route changes after the prefix, please
* use the routing mechanism of your app.
*
* @param legacyAppId The name of the old app to forward URLs from
* @param newAppId The name of the new app that handles the URLs now
* @param options Whether the prefix of the old app is kept to nest the legacy
* path into the new path
*/
forwardApp(
legacyAppId: string,
newAppId: string,
options: { keepPrefix: boolean } = { keepPrefix: false }
) {
this.forwards.push({ legacyAppId, newAppId, ...options });
}

/**
* Wires up listeners to handle mounting and unmounting of apps to
* the legacy angular route manager. Once all apps within the Kibana
* plugin are using the local route manager, this implementation can
* be switched to a more lightweight implementation.
*
* @param angularRouteManager The current `ui/routes` instance
*/
apply(angularRouteManager: UIRoutes) {
this.apps.forEach(app => {
const wrapperElementId = this.idGenerator();
angularRouteManager.when(matchAllWithPrefix(app), {
outerAngularWrapperRoute: true,
reloadOnSearch: false,
reloadOnUrl: false,
template: `<div style="height:100%" id="${wrapperElementId}"></div>`,
controller($scope: IScope) {
const element = document.getElementById(wrapperElementId)!;
(async () => {
const onUnmount = await app.mount({ core: npStart.core }, { element, appBasePath: '' });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')

Copy link
Contributor Author

@flash1293 flash1293 Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

$scope.$on('$destroy', () => {
onUnmount();
});
})();
},
});
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const newPath = `/${newAppId}${keepPrefix ? path : path.replace(legacyAppId, '')}`;
return `${newPath}?${search}`;
},
});
});
}
}

export const localApplicationService = new LocalApplicationService();
22 changes: 21 additions & 1 deletion src/legacy/ui/public/legacy_compat/angular_config.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ import { isSystemApiRequest } from '../system_api';

const URL_LIMIT_WARN_WITHIN = 1000;

function isDummyWrapperRoute($route: any) {
return (
$route.current && $route.current.$$route && $route.current.$$route.outerAngularWrapperRoute
);
}

export const configureAppAngularModule = (angularModule: IModule) => {
const newPlatform = npStart.core;
const legacyMetadata = newPlatform.injectedMetadata.getLegacyMetadata();
Expand Down Expand Up @@ -187,6 +193,9 @@ const $setupBreadcrumbsAutoClear = (newPlatform: CoreStart) => (
});

$rootScope.$on('$routeChangeSuccess', () => {
if (isDummyWrapperRoute($route)) {
return;
}
const current = $route.current || {};

if (breadcrumbSetSinceRouteChange || (current.$$route && current.$$route.redirectTo)) {
Expand Down Expand Up @@ -226,6 +235,9 @@ const $setupBadgeAutoClear = (newPlatform: CoreStart) => (
});

$rootScope.$on('$routeChangeSuccess', () => {
if (isDummyWrapperRoute($route)) {
return;
}
const current = $route.current || {};

if (badgeSetSinceRouteChange || (current.$$route && current.$$route.redirectTo)) {
Expand Down Expand Up @@ -270,6 +282,9 @@ const $setupHelpExtensionAutoClear = (newPlatform: CoreStart) => (
const $route = $injector.has('$route') ? $injector.get('$route') : {};

$rootScope.$on('$routeChangeStart', () => {
if (isDummyWrapperRoute($route)) {
return;
}
helpExtensionSetSinceRouteChange = false;
});

Expand All @@ -286,10 +301,15 @@ const $setupHelpExtensionAutoClear = (newPlatform: CoreStart) => (

const $setupUrlOverflowHandling = (newPlatform: CoreStart) => (
$location: ILocationService,
$rootScope: IRootScopeService
$rootScope: IRootScopeService,
$injector: any
) => {
const $route = $injector.has('$route') ? $injector.get('$route') : {};
const urlOverflow = new UrlOverflowService();
const check = () => {
if (isDummyWrapperRoute($route)) {
return;
}
// disable long url checks when storing state in session storage
if (newPlatform.uiSettings.get('state:storeInSessionStorage')) {
return;
Expand Down
4 changes: 3 additions & 1 deletion src/legacy/ui/public/routes/route_manager.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ import { ChromeBreadcrumb } from '../../../../core/public';

interface RouteConfiguration {
controller?: string | ((...args: any[]) => void);
redirectTo?: string;
redirectTo?: string | ((params: object, path: string, search: string) => string);
reloadOnSearch?: boolean;
reloadOnUrl?: boolean;
outerAngularWrapperRoute?: boolean;
resolve?: object;
template?: string;
k7Breadcrumbs?: (...args: any[]) => ChromeBreadcrumb[];
Expand Down