From c06eb06c309777f959d4fa2d72d9414b60eb928e Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Wed, 4 Dec 2019 10:13:14 +0100 Subject: [PATCH 1/5] Table of contents for conventions --- src/core/CONVENTIONS.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/core/CONVENTIONS.md b/src/core/CONVENTIONS.md index 786409614e6d94..a1d126db77db27 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 From 0c4bad5089fdd4d5f04fc4290a951d166b8e918c Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Fri, 6 Dec 2019 17:04:01 +0100 Subject: [PATCH 2/5] Add Core Conventions --- src/core/CORE_CONVENTIONS.md | 87 ++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 src/core/CORE_CONVENTIONS.md diff --git a/src/core/CORE_CONVENTIONS.md b/src/core/CORE_CONVENTIONS.md new file mode 100644 index 00000000000000..4c94a7d08d78e5 --- /dev/null +++ b/src/core/CORE_CONVENTIONS.md @@ -0,0 +1,87 @@ +# 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 (preferred) -- + /** + * @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) -- + export interface IUiSettingsClient { + /** Retrieve all settings */ + 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`. Deriving the public + > contract from the class is preferred over a separate interface. It + > reduces the duplication of types and doc comments are: a) located with + > the source code, b) included in our generated docs and c) visible when + > hovering over the type in a code editor. + +## 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. \ No newline at end of file From 100e474e75ff74eae960b4bc0fa8feb560bafb65 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Fri, 6 Dec 2019 17:17:24 +0100 Subject: [PATCH 3/5] Add Tests and mocks section --- src/core/CORE_CONVENTIONS.md | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/src/core/CORE_CONVENTIONS.md b/src/core/CORE_CONVENTIONS.md index 4c94a7d08d78e5..536805a761b930 100644 --- a/src/core/CORE_CONVENTIONS.md +++ b/src/core/CORE_CONVENTIONS.md @@ -1,3 +1,8 @@ +- [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 @@ -84,4 +89,27 @@ area of Core API's and does not apply to internal types. > 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. \ No newline at end of file + > churn when we know related API methods will be introduced. + +## 3. Tests and mocks + - 3.1 Declary 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. \ No newline at end of file From 9564676de5ee9b89b3a499c38ac4730cff674ee0 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Mon, 6 Jan 2020 14:31:09 +0100 Subject: [PATCH 4/5] Update src/core/CORE_CONVENTIONS.md Typo Co-Authored-By: Josh Dover --- src/core/CORE_CONVENTIONS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/CORE_CONVENTIONS.md b/src/core/CORE_CONVENTIONS.md index 536805a761b930..3327a068d7cb0a 100644 --- a/src/core/CORE_CONVENTIONS.md +++ b/src/core/CORE_CONVENTIONS.md @@ -92,7 +92,7 @@ area of Core API's and does not apply to internal types. > churn when we know related API methods will be introduced. ## 3. Tests and mocks - - 3.1 Declary Jest mocks with a temporary variable to ensure types are + - 3.1 Declare Jest mocks with a temporary variable to ensure types are correctly inferred ```ts @@ -112,4 +112,4 @@ area of Core API's and does not apply to internal types. > Why? Without the temporary variable, Jest types the `start` function as > `jest` and, as a result, doesn't typecheck the mock return - > value. \ No newline at end of file + > value. From 5e0632070600b7d2b474ab80a5dd1e2c86eb89ee Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Wed, 15 Jan 2020 14:26:14 +0100 Subject: [PATCH 5/5] Add pro's/con's for alternatives to private fields support --- src/core/CORE_CONVENTIONS.md | 43 ++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/src/core/CORE_CONVENTIONS.md b/src/core/CORE_CONVENTIONS.md index 3327a068d7cb0a..76f3be1595258d 100644 --- a/src/core/CORE_CONVENTIONS.md +++ b/src/core/CORE_CONVENTIONS.md @@ -33,7 +33,7 @@ area of Core API's and does not apply to internal types. prefixed with an 'I', to describe the public contract of the class. ```ts - // -- good (preferred) -- + // -- good (alternative 1) -- /** * @public * {@link UiSettingsClient} @@ -47,12 +47,16 @@ area of Core API's and does not apply to internal types. public getSettings(): { return this.settings; } }; - // -- good (alternative) -- - export interface IUiSettingsClient { + // -- 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 { @@ -62,11 +66,32 @@ area of Core API's and does not apply to internal types. ``` > Why? Classes' private members form part of their type signature making it - > impossible to mock a dependency typed as a `class`. Deriving the public - > contract from the class is preferred over a separate interface. It - > reduces the duplication of types and doc comments are: a) located with - > the source code, b) included in our generated docs and c) visible when - > hovering over the type in a code editor. + > 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 @@ -93,7 +118,7 @@ area of Core API's and does not apply to internal types. ## 3. Tests and mocks - 3.1 Declare Jest mocks with a temporary variable to ensure types are - correctly inferred + correctly inferred. ```ts // -- good --