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

Mock ES6 Classes in New Platform #33396

Closed
mshustov opened this issue Mar 18, 2019 · 4 comments
Closed

Mock ES6 Classes in New Platform #33396

mshustov opened this issue Mar 18, 2019 · 4 comments
Labels
Feature:New Platform stale Used to mark issues that were closed for being stale Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@mshustov
Copy link
Contributor

mshustov commented Mar 18, 2019

Description

We need to figure out the way to mock dependencies defined as ES6 Class. Our current approach implies we mock only external methods of the class, thus when we pass a mock to a function that expects real ES6 Class we get incompatibility error.

Example
https://github.com/elastic/kibana/pull/32999/files#diff-71ede491c56cdaa1766ec8a3a434cd36R38
If you remove a manual type casting you can see an error due to lack of class internals.
...missing the following properties from type 'UiSettingsClient': update$, saved$, api...

Related discussion

#32300 (comment)
#31486
microsoft/TypeScript#18499

Proposed solutions

mute error and wait for real private fields supported in TS 😄

toasts: (toastsServiceMock.createStartContract() as unknown) as ToastsStart,

Disadvantages No type check, it's not critical because actually we have a guarantee for public methods, as this type check is already made in https://github.com/elastic/kibana/pull/32999/files#diff-ef336fc230d3646d86f73486ed09ec05R22

Separate interface declaration and implementation

#32300 (comment)

// config.js
class ConfigService implements IConfigService {...
// server.js
class Server {
  constructor(configService: IConfigService, ...)

disadvantages introduces additional concept - a public interface, also forces to define all method types manually!

export interface IToastsStart {
  get$: () => Rx.Observable<Toast[]>;
  add: (toastOrTitle: ToastInput) => Toast;
  remove: (toast: Toast) => void;
  addSuccess: (toastOrTitle: ToastInput) => Toast;
  addWarning: (toastOrTitle: ToastInput) => Toast;
  addDanger: (toastOrTitle: ToastInput) => Toast;
}
// or more confusing syntax, but less verbose
export type IToastsStart = PublicMethodsOf<ToastsStart>;

the full example master...restrry:example-with-interface

make mocks to extend class implementation #32300 (comment)

disadvantages in the comment ^^

@mshustov mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Mar 18, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@mshustov
Copy link
Contributor Author

mshustov commented Mar 18, 2019

@elastic/kibana-platform thoughts?

@joshdover
Copy link
Contributor

I prefer defining explicit interfaces that can be implemented as classes or objects.

Though it's more verbose, it has one key advantage: the ability to separate the public interface from the internal one that may have more APIs. For instance, the ChromeService has APIs that are available to plugins and ones that are only meant to be consumed by other Core services. With the interface approach we can declare the public APIs explicitly and still avoid having to duplicate the internal ones:

/** @public */
export interface ChromeStart {
  setBrand(brand?: string): void;
}

/**
 * This can be grabbed via `ReturnType` since we don't need to generate pretty docs
 * @internal
 */
export type InternalChromeStart = UnrwapPromise<ReturnType<ChromeService['start']>>;

class ChromeService implements CoreService<void, ChromeStart> {
  public async start() {
    return {
      setBrand() {},
      /** Internal-only API */
      getComponent(): JSX.Element {
        return <div>Hi</div>;
      },
    };
  }
}

In light of #34416 needed to support good API docs, we need explicit interfaces anyways and this approach does not create any additional work.

@joshdover
Copy link
Contributor

@elastic/kibana-platform Just a reminder that we decided to come to a consensus on the solution here sooner than later. Any thoughts on what I proposed?

@rudolf rudolf mentioned this issue Dec 6, 2019
7 tasks
@joshdover joshdover added the stale Used to mark issues that were closed for being stale label Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform stale Used to mark issues that were closed for being stale Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

3 participants