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

Add ApplicationService Mounting #41007

Open
wants to merge 6 commits into
base: master
from

Conversation

@joshdover
Copy link
Member

commented Jul 12, 2019

Summary

Implements RFC-0004
Related to #18843
Depends on #41251

This adds support for UI applications in the New Platform. With this change, CoreSystem on the frontend now has a "legacy mode" and normal mode.

  • In legacy mode, the ApplicationService does not mount its router on the page and instead gives control to the LegacyService.
  • In normal mode, the ApplicationService mounts its router on the page and controls mounting individual UI apps. The LegacyService does import any legacy code, though it does still provide some data and features to mirror Legacy behaviors.

This mode is toggled by the legacy renderApp function, which serves the bootstrapping page and bundles. When rendering a UI app that is unknown to the legacy platform, this function will instead serve the "core-only" bundle which does not include any legacy code or applications.

New Platform applications currently have some caveats:

  • Telemetry banner will not show (need #41986 implemented)
  • No legacy code, including uiExport "hacks" are run when NP apps are running.
    • This means some features will not work on these pages, such as reporting's polling for completed jobs.
    • I've done an audit of all our uiExport hacks that need to be updated: #42984
  • Nav links for NP apps do not support "open in new window". To support this, we need EUI to support links with both href and onClick props (elastic/eui#1933)

Testing

To test the NP application service yourself, unzip these plugins into your plugins directory, run yarn kbn bootstrap, and start Kibana.

Alternatively, you can also start the functional test server with the config that loads some test applications:

node scripts/functional_tests_server.js --config test/plugin_functional/config.js

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine

This comment was marked as outdated.

Copy link

commented Jul 12, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

commented Jul 12, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch 2 times, most recently from 2b9d376 to e081514 Jul 12, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

commented Jul 12, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from e081514 to 85c36ec Jul 16, 2019

@joshdover joshdover added this to In progress in kibana-platform Jul 16, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

commented Jul 16, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from 85c36ec to 8c32728 Jul 24, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

commented Jul 24, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

commented Jul 29, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from 7ffe664 to e6c4287 Jul 30, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

commented Jul 30, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from e6c4287 to adcb6cc Jul 30, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

commented Jul 30, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from adcb6cc to 52ece80 Aug 1, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

commented Aug 1, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from 52ece80 to a521c54 Aug 1, 2019

@joshdover

This comment was marked as outdated.

Copy link
Member Author

commented Aug 2, 2019

retest

@elasticmachine

This comment was marked as outdated.

Copy link

commented Aug 2, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from a521c54 to 0c0fd8b Aug 2, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

commented Aug 2, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch 2 times, most recently from aed2a1c to 0908cfc Aug 2, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from 891c844 to 001be7f Aug 12, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 12, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from 001be7f to 0f30eb7 Aug 12, 2019

@epixa epixa referenced this pull request Aug 12, 2019
8 of 12 tasks complete

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from 0f30eb7 to fe51d34 Aug 12, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 12, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 13, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from c6cd14b to 3623d20 Aug 13, 2019

@joshdover

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Taking off for the week 🏖, I'll update this when I get back!

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 13, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from 3623d20 to 9eb2476 Aug 19, 2019

@joshdover

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

@restrry Addressed all comments!

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 19, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 19, 2019

@joshdover joshdover referenced this pull request Aug 20, 2019
@eliperelman
Copy link
Member

left a comment

LGTM, most of the comments are just nits. 👍

},
]
`);
Map {

This comment has been minimized.

Copy link
@eliperelman

eliperelman Aug 20, 2019

Member

Does this whitespace look strange to you?

This comment has been minimized.

Copy link
@joshdover

joshdover Aug 21, 2019

Author Member

When Jest writes a new snapshot to the file it messes these up sometimes, it kept happening so I stopped fighting it.

This comment has been minimized.

Copy link
@rudolf

rudolf Aug 21, 2019

Contributor

I also had a couple of fights with jest about this ;) it's a known issue: facebook/jest#8424

private readonly containerDiv = React.createRef<HTMLDivElement>();
private unmountFunc?: AppUnmount;

constructor(props: Props) {

This comment has been minimized.

Copy link
@eliperelman

eliperelman Aug 20, 2019

Member

I think this can be shortened to a class field:

- constructor(props: Props) {
-   super(props);
-   this.state = { appNotFound: false };
- }
+ state = { appNotFound: false };
// This needs to work with both href and onClick to support "open in new tab" correctly, however EUI
// does not current support this.
// https://github.com/elastic/eui/pull/1933
href: legacyMode || navLink.legacy ? navLink.href : undefined,

This comment has been minimized.

Copy link
@eliperelman

eliperelman Aug 20, 2019

Member

We can simplify this to only check the condition once via:

...(legacyMode || navLink.legacy
  ? { href: navLink.href }
  : { onClick: () => navigateToApp(navLink.id) }
),

This comment has been minimized.

Copy link
@joshdover

joshdover Aug 21, 2019

Author Member

I plan on fixing the caveat mentioned in the comment once EUI is upgraded. That should happen before this merges, so I'm just going to leave this as-is since it will change anyways.

src/legacy/ui/public/chrome/chrome.js Show resolved Hide resolved
hash: hash || appConfig.hash,
});

let appUrl;

This comment has been minimized.

Copy link
@eliperelman

eliperelman Aug 20, 2019

Member

Can be compacted via ternary to:

const appConfig = config.get(['apps', appName]);
const appUrl = getUrl.noAuth(
  config.get('servers.kibana'),
  appConfig
    ? { pathname: `${basePath}${appConfig.pathname}`, hash: hash || appConfig.hash }
    : { pathname: `${basePath}/app/${appName}` }
);

This comment has been minimized.

Copy link
@joshdover

joshdover Aug 21, 2019

Author Member

This doesn't work because config.get(['apps', appName]) throws an exception if it doesn't exist.

This comment has been minimized.

Copy link
@eliperelman

eliperelman Aug 21, 2019

Member

Aww, that's unfortunate.

joshdover added some commits Jul 26, 2019

@joshdover joshdover force-pushed the joshdover:app-service/mount branch from 776c03d to 7f36447 Aug 21, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 21, 2019

* application.register({
* id: 'my-app',
* async mount(context, params) {
* const { renderApp } = await import('./applcation');

This comment has been minimized.

Copy link
@rudolf

rudolf Aug 21, 2019

Contributor
Suggested change
* const { renderApp } = await import('./applcation');
* const { renderApp } = await import('./application');
*/
export interface App extends AppBase {
/**
* A mount function called when the user navigates to this app's `rootRoute`.

This comment has been minimized.

Copy link
@rudolf

rudolf Aug 21, 2019

Contributor

Can't find any references to rootRoute, was this meant to be renamed to appBasePath (I like the name much better :D)?

* @returns An unmounting function that will be called to unmount the application.
*/
mount(context: MountContext, targetDomElement: HTMLElement): Unmount | Promise<Unmount>;
mount(context: MountContext, params: AppMountParams): Unmount | Promise<Unmount>;

This comment has been minimized.

Copy link
@rudolf

rudolf Aug 21, 2019

Contributor

Whats the different between a context and params here and how do we decide what goes into which? Conceptually I've been thinking of context as everything stateful that your app could require, so this includes stateful libraries like core or plugins, but also just data like an html element or an appBasePath.

/**
* An ordinal used to sort nav links relative to one another for display.
*/
order?: number;

This comment has been minimized.

Copy link
@rudolf

rudolf Aug 21, 2019

Contributor

Why would we want order to be optional? If two or more apps don't specify an ordering then the ordering might not be guaranteed to be stable. I'm guessing if a non-unique order number is specified the ordering is affected by some underlying implementation detail like the order in which apps were registered, which in turn could be affected by the dependency graph?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.