[SR] Snapshot and restore plugin boilerplate

[jen-huang]

The beginning of snapshot and restore GUI!

This PR sets up the new plugin and includes some boilerplate client-side and server-side code. Since this plugin was created with new Kibana platform principles in mind, I am primarily looking for feedback with regards to that. You'll notice two sets of shim.ts and plugin.ts files, one used by client and the other for server.

@epixa, I ran into some difficulties with accessing core and plugins from child React components. I ended up creating a React context, AppContext, which is initialized when parent component is mounted, and referenced by child components using contextType so that core and plugins can be used in lifecycle methods. Feedback and/or suggestions for this would be super appreciated.

[elasticmachine]

Pinging @elastic/es-ui

[cjcenizal]

This is so 🔥. Thanks for leading the way on this! This will make building plugins compatible with the new platform so much easier (and converting existing ones). I think the AppContext mechanism you've created for sharing injected dependencies is a terrific solution. My initial idea was a service locator, but this is much more elegant!

I think it would make sense to move the repository_list and snapshot_list directories into home. This localizes these components with the page which will consume them, and the resulting tree-like folder structure will mirror the structure of the DOM and UI. This will also declutter the sections directory as we add more pages.

[cjcenizal]

Can we consume license type constants exported from common instead of hardcoding strings?

[cjcenizal]

Could this refer to a constant instead of using a hardcoded string?

[cjcenizal]

Can we internationalize this?

[jen-huang]

This one was tricky since I'm trying to avoid calling i18n outside of shim.ts. In the end, I changed this to be a function, getI18nName(), which is called when the plugin is initialized.

Also, this is called in the legacy world rather than plugin.ts to avoid the plugin knowing about server, which is needed by registerLicenseChecker.

@epixa, any thoughts on accessing i18n, or other core/plugin modules from constant files like this case?

[cjcenizal]

I always get a little concerned about creating config objects because I need to understand how they're consumed in order to understand how they "work". For example in this case I need to also read register_routes.ts to gain that context and understand how these objects work. But if we keep these as functions then I would have enough context to understand them on their own (theoretically we could also unit test, but in this case I guess that test wouldn't be very valuable 😄). What are your thoughts on this?

export const registerGetAll = router => {
  router.get('repositories', async (req: any, callWithRequest: any, responseToolkit: any) => {
    const repositoriesByName = await callWithRequest('snapshot.getRepository', {
      repository: '_all',
    });
    const repositories = Object.keys(repositoriesByName).map(name => {
      return {
        name,
        ...repositoriesByName[name],
      };
    });
    return repositories;
  });
}

[jen-huang]

I adjusted this to something similar to your suggestion - each handler is broken out into it's own named function, and registered registerRepositoriesRoutes(). I also added additional typings to enforce the signature of each handler function

[cjcenizal]

From looking at how some other plugins do it, I think we can use a few types here:

import { Legacy } from 'kibana';
import { Request, ResponseToolkit } from 'hapi';
import { CallClusterWithRequest } from 'src/legacy/core_plugins/elasticsearch';

handler: async (req: Legacy.Request, callWithRequest: CallClusterWithRequest, responseToolkit: ResponseToolkit) => {

// or this one... not sure why we have both Legacy.Request and Request
handler: async (req:Request, callWithRequest: CallClusterWithRequest, responseToolkit: ResponseToolkit) => {

[cjcenizal]

We can type server:

import { Server } from 'hapi';
export declare function createRouter(server: Server, pluginId: string, apiBasePath: string): Router;

[cjcenizal]

I believe we can type request per my comment above.

[epixa]

I think the legacy ES call stuff is all typed now: https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/elasticsearch/index.d.ts

[cjcenizal]

We can type server, per my comment above.

[epixa]

I recommend pulling the shimming logic into snapshot_restore/index.ts, so plugin.ts doesn't appear coupled to the legacy stuff at all. So you'd call createShim from the legacy init() and pass core and plugins as arguments to this start().

Over time you'll convert more of the shim stuff over to new platform services directly, and then eventually you'll just delete the legacy plugin definition entirely, and your new plugin definition won't really change along the way.

[epixa]

The react context approach makes a lot of sense in this case, I think. You might want to be more selective in passing down the features you want to use though, rather than just passing the whole arguments through.

It forces developers to make an explicit decision to take advantage of a new plugin or core feature somewhere deep within your code, which can help increase scrutiny around increased levels of coupling.

At least this is all TypeScript so it's easy to manage even if you do pass them through.

[epixa]

Like in my comment on the server, I think you should pull the shim logic out of the Plugin class and into the code that instantiates this and calls .start(). Keep the new code as pristine as possible, and let the legacy code handle the hackery.

[epixa]

Can you explain why this needs to be an angular route/controller rather than just being react like the rest of the plugin?

[jen-huang]

since the Kibana chrome is using Angular routing, we need to register the initial base path with Angular so it's accessible from the management menu. we then mount the React app inside it which handles the rest of internal stuff in React.

[epixa]

So if the management app itself was built using something like react router rather angular router, this wouldn't be necessary here, is that correct?

[epixa]

This is really awesome! I left a few comments, but overall this nailed the spirit of the migration guide, and I think it turned out well.

[jen-huang]

Thanks for the reviews @cjcenizal and @epixa! I've addressed your feedback.

I've fleshed out the typings to what I believe is a comprehensive enough point. I will add tests in the next pass when building out the actual features, so this PR is ready for another review by you two, or anyone else I have tagged. 😄

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[mshustov]

when this PR or #32746 finished we need to update typings here and below

[jen-huang]

thanks for the heads up 😄 I am happy to update these typings when that PR is merged. work with continue for this feature in subsequent PRs into the main feature branch, so it will be easy to update since there won't be any conflicts with master

[cchaos]

I know there aren't any actual styles in here (yet) but prefixes should just be 3 letters. I would change it now so its correct if people start adding styles.

[jen-huang]

thanks for that info, I've updated this to be ssr prefix instead

[bmcconaghy]

Looking pretty great Jen, nice work. I left a few comments, but nothing major.

[bmcconaghy]

leftover commented out code

[bmcconaghy]

why the or null here? seems like it should always be an Element.

[bmcconaghy]

more commented out code

[bmcconaghy]

wondering if maybe we want to start using function components + hooks instead of class ones.

[jen-huang]

I'd like to address this with the next pass when actual features start getting built out, it's hard for me to formulate an opinion for function components with hooks instead of lifecycles when the components don't do much yet 😁 I'll try function components + hooks for new components, and refactor these class ones if it goes well!

[bmcconaghy]

we should check elem for null and only render if not null I think, then elem above can remove | null

[bmcconaghy]

routing seems like maybe not the best name for this function, since it has unmount logic, which seems unrelated to routing, more like rendering to me.

[jen-huang]

good point, I'll move unmounting logic out of shim, and keep routing for just routing

[bmcconaghy]

wondering if this should be broken out into rendering

[bmcconaghy]

there's a type for $http ng.IHttpService

[jen-huang]

Thanks for the feedback, all! I've addressed the latest batch of em.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[epixa]

One detail that I don't think is obvious is that all code originating from the plugin shim "hack" file will be bundled globally into all application bundles. That's the nature of hacks - they are global across all applications.

The consequence of this is potential for rapid bloat of our common bundle size as more plugins shim more stuff. This is in contrast to most other uiExport-based functionality, which is only included in bundles when it is actually used by that application. In otherwords, your snapshot code will be loaded by users even when they're accessing things like timelion, ml, and apm.

This will also be true of plugin definitions in the new platform, so it was an intentional design decision.

The way we combat this problem is by having plugin developers use async imports to lazy load code only when it is necessary. This can technically be done in any extension point that allows your plugin to register an async function, and as a plugin author you should carefully consider where it makes the most sense. Too many async imports will result in poor performance as the browser has to load a ton of JavaScript files. Poorly placed async imports can potentially result in poor performance as the browser might load a bunch of JavaScript files one at a time.

Registering management sections is a great place for using an async import, and I would expect this to be the expectation of all management sections in the future. Each section potentially has a ton of code powering it, but you only need to load that code when you actually navigate to that section. Any time we see this sort of situation, an async import is probably a good idea.

Taking advantage of this requires an understanding of the problem from both sides. The management app itself must provide an extension point that allows an async function to load the section. Then the downstream plugin needs to put all of its section loading logic behind an async import. It would be pretty easy for you to do the latter in this pull request, but I'm not familiar with how management handles registering of sections, and I'm not sure if it would support an async function in its current form.

In my mind, I'd expect this to look something like:

// inside start() of plugin.js
management.getCategory('elasticsearch').registerSection({
  id: 'snapshots',
  name: 'Snapshots',
  async render(domElement) {
    const { loadSection } = await import('./section');
    return loadSection(domElement, /* any core/plugin stuff you need in the section itself */);
  }
});

// section.js
// most of the logic and imports related to the snapshots section in management
// would originate here, which ensures they aren't loaded until a user navigates here
export function loadSection(domElement, ...) {
  // would render the section into the domElement container
  return function unloadSection() {
    // invoked by management app when navigating away from section
    // would destroy/cleanup anything in the section that wouldn't automatically be
    // cleaned up when the management app killed the dom element
  }
}

I know that's not how it looks today, but perhaps the chief goal of hiding the bulk of the section logic behind an async import is technically possible with the existing API? I don't know enough about the existing API to say for sure.

[jen-huang]

Thanks for the insights, @epixa. I agree that it would be great to use async imports for management apps. Unfortunately, the management section registry right now is not that sophisticated with regards to rendering/mounting apps; it really only maintains a list of links with a few config options.

Not hearing any other objections, I'm going to merge this into the feature branch 🙂

[cjcenizal]

Wow, so many types! Code LGTM. Awesome work.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request