[skip-ci] Core conventions #52397

rudolf · 2019-12-06T16:18:55Z

Summary

Adds conventions adopted by the Platform team for src/core.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-12-06T16:18:58Z

Pinging @elastic/kibana-platform (Team:Platform)

rudolf · 2019-12-06T16:19:44Z

src/core/CORE_CONVENTIONS.md

+   > 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.


Discussed here #46668 (comment) and here #33396

rudolf · 2019-12-06T16:22:07Z

src/core/CORE_CONVENTIONS.md

+   };
+
+   // -- good (alternative) --
+   export interface IUiSettingsClient {


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.

Pros having explicit interfaces:
There is a difference in that an interface is a source of truth when we define it explicitly. it means that we have control of what properties are a part of a public contract.
Better tooling, as there are no intermediate types. Whenever I click Go to definition on SavedObjectsClientContract it navigates to Pick type.

Cons:
It creates an additional burden to define contracts manually.
Separates docs for an interface and implementation.

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).

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 PublicContractOf). I see real private fields as the best solution, source code and docs live next to each other, no intermediate types, no type duplication. When Kibana adopts TS 3.8 would we still prefer a separate interface?

When Kibana adopts TS 3.8 would we still prefer a separate interface?

Don't think so. IIRC public/private field separation was the main reason for an explicit interface introduction.

I think it could be slightly easier to migrate from PublicContractOf to private fields, but it wouldn't be that much work to migrate away from separate interfaces either.

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.

Works for me. Agreed that in most use-cases real private fields works well.

rudolf · 2019-12-06T16:22:28Z

src/core/CORE_CONVENTIONS.md

+   > 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


Discussed here: https://github.com/elastic/kibana/pull/40554/files#r303390356

rudolf · 2019-12-06T16:24:10Z

src/core/CORE_CONVENTIONS.md

+   > 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


Discussed here: #48431 (comment)

rudolf · 2019-12-06T16:25:15Z

@elastic/kibana-platform Are there other conventions you can think of that's worth documenting as part of this PR?

src/core/CORE_CONVENTIONS.md

pgayvallet · 2019-12-07T16:03:06Z

src/core/CORE_CONVENTIONS.md

+   > 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.


joshdover · 2019-12-18T19:31:53Z

src/core/CORE_CONVENTIONS.md

+   };
+
+   // -- good (alternative) --
+   export interface IUiSettingsClient {


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).

src/core/CORE_CONVENTIONS.md

Typo Co-Authored-By: Josh Dover <me@joshdover.com>

* 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 <me@joshdover.com> * Add pro's/con's for alternatives to private fields support Co-authored-by: Josh Dover <me@joshdover.com>

rudolf added 3 commits December 6, 2019 17:05

Table of contents for conventions

c06eb06

Add Core Conventions

0c4bad5

Add Tests and mocks section

100e474

rudolf added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Dec 6, 2019

rudolf commented Dec 6, 2019

View reviewed changes

rudolf marked this pull request as ready for review December 6, 2019 16:24

rudolf requested a review from a team as a code owner December 6, 2019 16:24

rudolf commented Dec 6, 2019

View reviewed changes

src/core/CORE_CONVENTIONS.md Show resolved Hide resolved

pgayvallet reviewed Dec 7, 2019

View reviewed changes

joshdover self-requested a review December 17, 2019 15:34

joshdover reviewed Dec 18, 2019

View reviewed changes

rudolf and others added 2 commits January 6, 2020 13:33

Merge branch 'master' into core-conventions

8f04088

Update src/core/CORE_CONVENTIONS.md

9564676

Typo Co-Authored-By: Josh Dover <me@joshdover.com>

mshustov approved these changes Jan 13, 2020

View reviewed changes

joshdover self-requested a review January 14, 2020 16:09

joshdover approved these changes Jan 14, 2020

View reviewed changes

rudolf mentioned this pull request Jan 15, 2020

Expose Core API's as classes using Typescript's "ES Private fields" #54906

Closed

rudolf added 2 commits January 15, 2020 14:26

Add pro's/con's for alternatives to private fields support

5e06320

Merge branch 'master' into core-conventions

6bf7f10

rudolf merged commit d10d628 into elastic:master Jan 15, 2020

rudolf deleted the core-conventions branch January 15, 2020 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[skip-ci] Core conventions #52397

[skip-ci] Core conventions #52397

rudolf commented Dec 6, 2019

elasticmachine commented Dec 6, 2019

rudolf Dec 6, 2019 •

edited

Loading

rudolf Dec 6, 2019

mshustov Dec 9, 2019

joshdover Dec 18, 2019

rudolf Jan 6, 2020

mshustov Jan 7, 2020

rudolf Jan 7, 2020

joshdover Jan 14, 2020

rudolf Dec 6, 2019

rudolf Dec 6, 2019

rudolf commented Dec 6, 2019

pgayvallet Dec 7, 2019

joshdover Dec 18, 2019

[skip-ci] Core conventions #52397

[skip-ci] Core conventions #52397

Conversation

rudolf commented Dec 6, 2019

Summary

Checklist

For maintainers

elasticmachine commented Dec 6, 2019

rudolf Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudolf commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudolf Dec 6, 2019 •

edited

Loading