From d10d6286ee526c0779eaa9420d9cc36d3cc05e85 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Wed, 15 Jan 2020 14:29:50 +0100 Subject: [PATCH] [skip-ci] Core conventions (#52397) * Table of contents for conventions * Add Core Conventions * Add Tests and mocks section * Update src/core/CORE_CONVENTIONS.md Typo Co-Authored-By: Josh Dover * Add pro's/con's for alternatives to private fields support Co-authored-by: Josh Dover --- src/core/CONVENTIONS.md | 8 +- src/core/CORE_CONVENTIONS.md | 140 +++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+), 1 deletion(-) create mode 100644 src/core/CORE_CONVENTIONS.md diff --git a/src/core/CONVENTIONS.md b/src/core/CONVENTIONS.md index 18f82766bdbc1..61c5d5b076a44 100644 --- a/src/core/CONVENTIONS.md +++ b/src/core/CONVENTIONS.md @@ -1,6 +1,12 @@ # Kibana Conventions -- [Plugin Structure](#plugin-structure) +- [Kibana Conventions](#kibana-conventions) + - [Plugin Structure](#plugin-structure) + - [The PluginInitializer](#the-plugininitializer) + - [The Plugin class](#the-plugin-class) + - [Applications](#applications) + - [Services](#services) + - [Usage Collection](#usage-collection) ## Plugin Structure diff --git a/src/core/CORE_CONVENTIONS.md b/src/core/CORE_CONVENTIONS.md new file mode 100644 index 0000000000000..76f3be1595258 --- /dev/null +++ b/src/core/CORE_CONVENTIONS.md @@ -0,0 +1,140 @@ +- [Core Conventions](#core-conventions) + - [1. Exposing API Types](#1-exposing-api-types) + - [2. API Structure and nesting](#2-api-structure-and-nesting) + - [3. Tests and mocks](#3-tests-and-mocks) + +# Core Conventions + +This document contains conventions for development inside `src/core`. Although +many of these might be more widely applicable, adoption within the rest of +Kibana is not the primary objective. + +## 1. Exposing API Types +The following section applies to the types that describe the entire surface +area of Core API's and does not apply to internal types. + + - 1.1 All API types must be exported from the top-level `server` or `public` + directories. + + ```ts + // -- good -- + import { IRouter } from 'src/core/server'; + + // -- bad -- + import { IRouter } from 'src/core/server/http/router.ts'; + ``` + + > Why? This is required for generating documentation from our inline + > typescript doc comments, makes it easier for API consumers to find the + > relevant types and creates a clear distinction between external and + > internal types. + + - 1.2 Classes must not be exposed directly. Instead, use a separate type, + prefixed with an 'I', to describe the public contract of the class. + + ```ts + // -- good (alternative 1) -- + /** + * @public + * {@link UiSettingsClient} + */ + export type IUiSettingsClient = PublicContractOf; + + /** internal only */ + export class UiSettingsClient { + constructor(private setting: string) {} + /** Retrieve all settings */ + public getSettings(): { return this.settings; } + }; + + // -- good (alternative 2) -- + export interface IUiSettingsClient { + /** Retrieve all settings */ + public getSettings(): string; + } + + export class UiSettingsClient implements IUiSettingsClient { + public getSettings(): string; + } + + // -- bad -- + /** external */ + export class UiSettingsClient { + constructor(private setting: string) {} + public getSettings(): { return this.settings; } + } + ``` + + > Why? Classes' private members form part of their type signature making it + > impossible to mock a dependency typed as a `class`. + > + > Until we can use ES private field support in Typescript 3.8 + > https://github.com/elastic/kibana/issues/54906 we have two alternatives + > each with their own pro's and cons: + > + > #### Using a derived class (alternative 1) + > + > Pro's: + > - TSDoc comments are located with the source code + > - The class acts as a single source of type information + > + > Con's: + > - "Go to definition" first takes you to where the type gets derived + > requiring a second "Go to definition" to navigate to the type source. + > + > #### Using a separate interface (alternative 2) + > Pro's: + > - Creates an explicit external API contract + > - "Go to definition" will take you directly to the type definition. + > + > Con's: + > - TSDoc comments are located with the interface not next to the + > implementation source code. + > - Creates duplicate type information between the interface and + > implementation class. + +## 2. API Structure and nesting + - 2.1 Nest API methods into their own namespace only if we expect we will be + adding additional methods to that namespace. + + ```ts + // good + core.overlays.openFlyout(...); + core.overlays.openModal(...); + core.overlays.banners.add(...); + core.overlays.banners.remove(...); + core.overlays.banners.replace(...); + + // bad + core.overlays.flyouts.open(...); + core.overlays.modals.open(...); + ``` + + > Why? Nested namespaces should facilitate discovery and navigation for + > consumers of the API. Having namespaces with a single method, effectively + > hides the method under an additional layer without improving the + > organization. However, introducing namespaces early on can avoid API + > churn when we know related API methods will be introduced. + +## 3. Tests and mocks + - 3.1 Declare Jest mocks with a temporary variable to ensure types are + correctly inferred. + + ```ts + // -- good -- + const createMock => { + const mocked: jest.Mocked = { + start: jest.fn(), + }; + mocked.start.mockReturnValue(createStartContractMock()); + return mocked; + }; + // -- bad -- + const createMock = (): jest.Mocked => ({ + start: jest.fn().mockReturnValue(createSetupContractMock()), + }); + ``` + + > Why? Without the temporary variable, Jest types the `start` function as + > `jest` and, as a result, doesn't typecheck the mock return + > value.