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 New Platform conventions doc #39542

Merged
merged 1 commit into from
Jul 11, 2019
Merged

Conversation

joshdover
Copy link
Contributor

Summary

This is a WIP document aimed at collecting all the conventions we're adopting for developing plugins in the New Platform.

Please feel free to contribute more by pushing to this branch.

View Rendered

[skip-ci]

@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform Team:AppArch release_note:skip Skip the PR/issue when compiling release notes labels Jun 24, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

export class MyPlugin implements Plugin {
public setup() { ... }
public start() { ... }
public stop() { ... }
Copy link
Member

Choose a reason for hiding this comment

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

Are there "fairly stable" types defined for the arguments passed to these lifecycle methods (are they called lifecycle methods? idk)? It'd be fantastic to have the arguments and their types defined and linked to from here so we can get a sense of what we'll be doing in these methods. Thanks for this work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes there are types for this. You'll want to use the CoreSetup and CoreStart types from either src/core/public or src/core/server. By making your class implement the Plugin interface, you'll get an error if you accept any other type. Unfortunately, type interference doesn't assign the types automatically, so you'll have to do it yourself:

import { CoreSetup, CoreStart, Plugin } from '../../src/core/public';
export class MyPlugin implements Plugin {
  public setup(core: CoreSetup) { ... }
  public start(core: CoreStart) { ... }
  public stop() { ... } // does not accept any params at this time
}

These types are fairly stable, though we are still working on removing some from public's CoreSetup that should only be present in CoreStart. Other than that, the only major changes should be new services that are added.

Copy link
Member

Choose a reason for hiding this comment

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

This is perfect! I'd just use that example in the conventions doc so folks can track down those Core* types if they are curious about what gets passed to those methods. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with additional detail!

@jasonrhodes
Copy link
Member

This is Very Good 👏

@streamich
Copy link
Contributor

I agree that a summary of our plugin folder structure e.g. CONVENTIONS.md is very useful to have. Especially for somebody who wants to start a new plugin, so they don't have to reinvent the folder structure from scratch. And, in general, it would be useful for anyone doing plugins.

What I am not sure though, is if Core is the best place for such a document. Core could have its own CONVENTIONS.md that would summarize how core services are built, how they are tested, whether they have alternative in-memory implementation or not, etc. in the proposed src/core/CONVENTIONS.md file.

For the conventions of the internals of plugins themseves, it feels natural to have it in src/plugins/CONVENTIONS.md or src/plugins/kibana_utils/docs/Conventions.md (or wherever App Architecture team will keep its documentation) and set App Architecture team as an owner of that document.

@joshdover
Copy link
Contributor Author

What I am not sure though, is if Core is the best place for such a document.

I agree, most developers are not going to be looking here. The reason I put it here for now is that we plan on coming up with a complete strategy on how we want to structure API documentation, developer guides, and examples for NP development.

The Migration Guide, API Docs, and this document are the first pieces of content we have for that. I figured we keep it all together for now and until we decide where this content will live long-term.

@streamich
Copy link
Contributor

Few initial thoughts:

1. I'm not sure if there will be any plugin with multiple applications, especially as I believe @epixa was talking about actually reducing the number of icons in the left sidebar, but If a plugin has multiple apps, an extra "applications" folder is good:

my_plugin
├── public
│   ├── applications
│   │   ├── my_app1
│   │   │   └── components
│   │   ├── my_app2
│   │   │   └── components

Though, those multiple apps may want to share components between them:

my_plugin
├── public
│   ├── components
│   ├── applications
│   │   ├── my_app1
│   │   └── my_app2

But if the plugin has only one app (I guess most/all plugins will be like that?), I think it should be able to just have a single "application" folder:

my_plugin
├── public
│   ├── components
│   ├── application
│   │   └── App.tsx

I did a quick check; currently, it looks like most X-pack plugins just have an app.tsx file in "public":

my_plugin
├── public
│   ├── components
│   └── app.tsx

I guess then we need a reason for having application folder at all. Thoughts?


2. We will soon have Storybook available for all plugins, so the "components" folder could have a structure.

Components that are not connected to state (i.e. Redux, react-router, Core, etc.) could be in "components" folder and each of them can have a Storybook story.

Storybook stories are called "examples" in Canvas app, so below I call them "examples".

my_plugin
├── public
│   ├── components
│   │   ├── new_index_pattern_form
│   │   │   ├── __examples__
│   │   │   │   └── new_index_pattern_form.examples.tsx
│   │   │   └── new_index_pattern_form.tsx

Now, where do we put "connected" components (i.e. the ones with Redux, react-router, Core and other state injected)? There are a number of conventions in React world for hat. One way is to have a symmetric "containers" folder:

my_plugin
├── public
│   ├── components
│   │   └── new_index_pattern_form
│   ├── containers
│   │   └── new_index_pattern_form

Where components in "containers" folder import components from "components" folder and connect them to state.

Another pattern—which I prefer better—is instead of having two symmetric folders have everything that is related to a component in a single folder. For example, each component could have "component" and "connected" files—"component" is the one that can be rendered in Storybook and "connected" is the one that connects "component" to state:

my_plugin
├── public
│   ├── components
│   │   ├── new_index_pattern_form
│   │   │   ├── connected.tsx
│   │   │   └── component.tsx

That way everything related to a component we can have in a single folder, including tests, stories, CSS; and—as we will be using dynamic imports—sizeable components could also have lazy.tsx file that loads that component dynamically:

// lazy.tsx
import { lazy } from 'react';

export default lazy(() => import('./connected'));

To summarize, it could look something like this:

my_plugin
└── public
    └── components
        └── new_index_pattern_form
            ├── __examples__
            │   ├── __snapshots__
            │   └── examples.tsx
            ├── styles.css
            ├── connected.tsx
            ├── connected.test.tsx
            ├── component.tsx
            ├── index.tsx
            ├── README.md
            └── lazy.tsx

3. We were just recently discussing in a team how an extra layer of folders in data plugin inside ./public is a bit awkward.

data
├── public
│   ├── expressions
│   │   ├── expressions_service.ts
│   │   ├── index.ts
│   ├── filter
│   ├── index_patterns
│   ├── query
│   ├── search
│   ├── index.ts

That maybe we want to split those 5 folders into 5 plugins instead. (I believe they are independent anyways.) Now, if we introduce another "services" folder layer it would look like this:

data
├── public
│   ├── services
│   │   ├── expressions
│   │   │   ├── expressions_service.ts
│   │   │   ├── index.ts
│   │   ├── filter
│   │   ├── index_patterns
│   │   ├── query
│   │   ├── search
│   │   ├── index.ts
│   ├── index.ts

It is nice at the first glance. But you will probably have index.ts in every folder level where everything is re-exported. And data plugin will always be "services" plugin I believe (unless we split it into plugins and there will be no data plugin at all), so the public folder will be just a Java-like folder, which just has services folder inside (and index.tsx).

@streamich
Copy link
Contributor

streamich commented Jun 26, 2019

4. Something like below can be added about readme:

Each plugin must have a README.md file with at least a title and a paragraph describing the plugin.


5. Plugins could optionally have ./docs folder where more detailed documentation and examples live.

@jasonrhodes
Copy link
Member

I'm not sure if there will be any plugin with multiple applications

Infra already has two "applications" inside of one plugin (Infra Metrics and Logs). I like having a single applications folder convention to drive home the difference between a plugin and an app. Plugins that have no applications can omit that folder as another nice little sign of intention, etc.

the "components" folder could have a structure

I think a general set of conventions is a great idea, but I also think trying to dictate the folder structures of apps will eventually be seen as going a little too far. I think teams should probably figure that out for themselves, in my opinion—it's a fine line but trying to avoid crossing over from "how a Kibana plugin is generally structured" to "how React apps should be structured".

src/core/CONVENTIONS.md Outdated Show resolved Hide resolved
@lukeelmers lukeelmers mentioned this pull request Jul 3, 2019
6 tasks
@lukeelmers
Copy link
Member

Couple other areas that I wonder if we should include here:

  • Do we recommend (not require) that people include "service definition" files that mirror the lifecycle methods of the plugin definitions? Core does this, App Arch does this, and so far I think it has made it much easier to reason about how a plugin works in the NP.
  • Do we provide tips for managing static exports & common code?

it's a fine line but trying to avoid crossing over from "how a Kibana plugin is generally structured" to "how React apps should be structured".

100% agree with @jasonrhodes on this. Ask 5 devs about the "right" way to structure a React app, and you'll get 5 different answers.

I think @joshdover's approach here strikes a good balance: A clear separation of applications and services is useful IMHO, but what people want to do inside their apps/services shouldn't matter to the rest of Kibana. Some teams that develop multiple plugins may want to establish their own conventions that they agree upon, but realistically enforcing this across all of Kibana doesn't scale.

To me, there are two things we get out of this effort:

  1. Devs who are not familiar with a plugin's code can more easily navigate to understand the apps/services a plugin exposes. (This will be solved in the long term by a more comprehensive dev docs solution)
  2. Devs who are planning the migration of their plugin to the new platform have some code snippets that they can copy/paste as they start the process.

@joshdover
Copy link
Contributor Author

Responses to @streamich

I guess then we need a reason for having application folder at all. Thoughts?

I think having the directory is still helpful even if it's a single application. I believe having a small number of items in the top-level of the directory to be helpful for understanding a plugin's contents.

I've updated this to show that if you only have a single application (a common case), you can just call this directory application.

But you will probably have index.ts in every folder level where everything is re-exported. And data plugin will always be "services" plugin I believe

I think this is a feature, not a bug. When I open the data directory, I find it useful that the only directory in there is services. It makes it super clear that this is a "under-the-hood" plugin that maybe I should be leveraging rather than a UI app.

  1. Something like below can be added about readme:

    Each plugin must have a README.md file with at least a title and a paragraph describing the plugin.

  2. Plugins could optionally have ./docs folder where more detailed documentation and examples live.

One thing the New Platform project wants to accomplish is a detailed documentation for 3rd party developers. This means extracting docs into the AsciiDoctor pipeline we use for all other documentation on elastic.co.

Though we haven't worked on this yet, the way I see this working is extracting TSDocs from code to generate asciidoc files. We still need to determine how more "prose-like" documentation (user guides, examples) should fit into this.

One way we could do this is still via TSDocs, leveraging the @remarks, @packageDocumentation, and @example directives inside comment blocks.

For example, instead of a README, each plugin could have a top-level documentation block in their index.ts file:

/**
 * Data plugin for data fetching.
 * 
 * @remarks
 * Lots of detail about how this thing works.
 * 
 * @example
 * Here's a code example:
 * ```ts
 * data.doTheThing()
 * ```
 * 
 * @example
 * Here's a second example:
 * ```ts
 * data.doTheOtherThing()
 * ```
 *
 * @packageDocumentation
 */

It may turn out that this is not very practical and we need to supplement it with raw asciidocs file in the plugin directory. Until we build this tooling, I'm hesitant to specify how this should be done. Either way can be easily changed in the future.


Responses to @lukeelmers

Do we recommend (not require) that people include "service definition" files that mirror the lifecycle methods of the plugin definitions? Core does this, App Arch does this, and so far I think it has made it much easier to reason about how a plugin works in the NP.

Good idea, I've added a section for this.

Do we provide tips for managing static exports & common code?

I don't have strong opinions on this other maybe adding an exports directory. Do you have any input here?

it's a fine line but trying to avoid crossing over from "how a Kibana plugin is generally structured" to "how React apps should be structured".

100% agree with @jasonrhodes on this. Ask 5 devs about the "right" way to structure a React app, and you'll get 5 different answers.

I think @joshdover's approach here strikes a good balance: A clear separation of applications and services is useful IMHO, but what people want to do inside their apps/services shouldn't matter to the rest of Kibana. Some teams that develop multiple plugins may want to establish their own conventions that they agree upon, but realistically enforcing this across all of Kibana doesn't scale.

I think getting into how applications should be structured is a bit too granular for a convention. The goal of having a consistent structure is to help a developer unfamiliar with your plugin be able to open your plugin directory and get a good sense of what are the primary contents of your plugin.

The details of how it's implemented will drive the shape of the directory tree, and I don't want to specify how your plugin should be built at that level of detail.

In spirit of this, I've removed the "components" directory from applications to let plugin developers decide how to shape this.

@joshdover joshdover marked this pull request as ready for review July 9, 2019 15:11
@joshdover joshdover requested a review from a team as a code owner July 9, 2019 15:11
Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

couple of nits

src/core/CONVENTIONS.md Outdated Show resolved Hide resolved
src/core/CONVENTIONS.md Outdated Show resolved Hide resolved
@lukeelmers
Copy link
Member

Do we provide tips for managing static exports & common code?

I don't have strong opinions on this other maybe adding an exports directory. Do you have any input here?

Yeah I brought it up because it is something I'm currently thinking about. I'll follow up with you on it once I have something more concrete. I don't think it needs to stop this PR from moving forward though; we can always supplement the material here later.

@joshdover joshdover merged commit 70a979a into elastic:master Jul 11, 2019
@joshdover joshdover deleted the np/conventions branch July 11, 2019 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants