-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[skip-ci] Core conventions #52397
[skip-ci] Core conventions #52397
Changes from 3 commits
c06eb06
0c4bad5
100e474
8f04088
9564676
5e06320
6bf7f10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
- [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 (preferred) -- | ||
/** | ||
* @public | ||
* {@link UiSettingsClient} | ||
*/ | ||
export type IUiSettingsClient = PublicContractOf<UiSettingsClient>; | ||
rudolf marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** internal only */ | ||
export class UiSettingsClient { | ||
constructor(private setting: string) {} | ||
/** Retrieve all settings */ | ||
public getSettings(): { return this.settings; } | ||
}; | ||
|
||
// -- good (alternative) -- | ||
export interface IUiSettingsClient { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if there are cases where we prefer to have a dedicated interface instead of inferring a type from a class? If we don't have good examples of where this would be better, I can remove this from the conventions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pros having explicit interfaces: Cons: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I prefer explicit interfaces. I'd rather be deliberate about choosing to expose a property or method rather than allowing it to happen without notice (though the api-extractor tooling may make it more obvious). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not completely convinced by the benefit of a separate interface. Adding a public method to an interface doesn't make it more obvious that it will change the API contract than adding a public method to a class. It seems like typescript support for private named fields will land in February https://github.com/microsoft/TypeScript/wiki/Roadmap#38-february-2020. If classes support real private fields/methods it will remove the need for an intermediate type (whether an explicit interface or There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Don't think so. IIRC public/private field separation was the main reason for an explicit interface introduction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it could be slightly easier to migrate from Since we're hopefully adopting a new convention soon, it probably doesn't matter too much that we have two alternatives. I can flesh out the comment section to mention the pro's and cons of both conventions and mention that we're hoping to adopt private fields once it's available. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Works for me. Agreed that in most use-cases real private fields works well. |
||
/** 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed here: #48431 (comment) |
||
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 Declary Jest mocks with a temporary variable to ensure types are | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed here: https://github.com/elastic/kibana/pull/40554/files#r303390356
rudolf marked this conversation as resolved.
Show resolved
Hide resolved
|
||
correctly inferred | ||
|
||
```ts | ||
// -- good -- | ||
const createMock => { | ||
const mocked: jest.Mocked<IContextService> = { | ||
start: jest.fn(), | ||
}; | ||
mocked.start.mockReturnValue(createStartContractMock()); | ||
return mocked; | ||
}; | ||
// -- bad -- | ||
const createMock = (): jest.Mocked<ContextServiceContract> => ({ | ||
start: jest.fn().mockReturnValue(createSetupContractMock()), | ||
}); | ||
``` | ||
|
||
> Why? Without the temporary variable, Jest types the `start` function as | ||
> `jest<any, any>` and, as a result, doesn't typecheck the mock return | ||
> value. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed here #46668 (comment) and here #33396