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

Add ContextService #40554

Closed
wants to merge 2 commits into from
Closed

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Jul 8, 2019

Summary

This adds a generic implementation for leveraging the Handler Context pattern.

For now this is exposed as an internal-only service that Core services can use to add context registration and context construction to their service. For example, this will be used in the ApplicationService like so:

export class ApplicationService {
  private readonly apps$ = new BehaviorSubject<App[]>([]);

  public setup() {
    return {
      registerApp: (app: App) =>
        this.apps$.next([...this.apps$.value, app]),
      registerContext: (contextName, provider) => 
        this.mountContext.register(contextName, provider, plugin),
    };
  }

  public async start({ context }) {
    const mountContext = context.createContextContainer<ApplicationContext>();
    
    // use mountContext.createContext(); when mounting an application

    return {
      registerContext: mountContext.register
    };
  }
}

To do in future PRs:

  • Create a pattern for exposing plugin-scoped service contracts for core services
  • Create a pattern for exposing plugin-scoped plugin contracts for plugins
  • Expose this service to plugins
  • Add a parallel service to the server-side

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@joshdover joshdover marked this pull request as ready for review July 9, 2019 22:18
@joshdover joshdover requested a review from a team as a code owner July 9, 2019 22:18
@joshdover joshdover added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.4.0 labels Jul 9, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover joshdover mentioned this pull request Jul 12, 2019
5 tasks
Copy link
Contributor

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! Left a couple small nits.

src/core/public/context/context.ts Outdated Show resolved Hide resolved
src/core/public/context/context.ts Outdated Show resolved Hide resolved
};

type ContextServiceContract = PublicMethodsOf<ContextService>;
const createMock = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I keep beating a dead horse on this one, but do we know why TypeScript doesn't infer this correctly?

const createMock = (): jest.Mocked<ContextServiceContract> => ({
  setup: jest.fn().mockReturnValue(createSetupContractMock()),
});

It seems strange that we need to create temporary throw-away variables to satisfy some problem with typing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @restrry knows more. I believe it's a fixable problem in DefinitelyTyped but not sure if it's been fixed yet?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jest doesn't know which function it mocks. if we want to have type safety, we should provide types manually
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/jest/index.d.ts#L120-L127
There are several options

  • define a type manually for every function
type Arguments<T> = T extends (...args: infer U) => any ? U : never;
//...
const mock = {
    start: jest
      .fn<
        ReturnType<typeof createStartContractMock>,
        Arguments<typeof createStartContractMock>
      >()
      .mockReturnValue(createStartContractMock),
}
  • or define a result type
const mock: jest.Mocked<ContextServiceContract> = {
    start: jest.fn(),
}
// and now, knowing the result type, jest can check whether a passed value satisfies the type
mock.start.mockImplementation(createStartContractMock);

when you declare a type as:

const createMock = (): jest.Mocked<ContextServiceContract> => ({
  setup: jest.fn().mockReturnValue(createSetupContractMock()),
});

TS uses this declaration https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/jest/index.d.ts#L123
and infers a type as jest.fn<any, any>
2019-07-15_13-23-10
Thus, any value passed as mockReturnValue or mockImplementation satisfies this type.

probably there is a better solution I'm not aware of.

src/core/public/context/context.ts Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

src/core/public/context/context.test.ts Outdated Show resolved Hide resolved
src/core/public/context/context.ts Outdated Show resolved Hide resolved

// If the provider is not from a plugin, give access to the entire
// context built so far (this is only possible for providers registered
// by the service owner).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never met service owner terminology in our code so far. is it core service?
If no, what if a plugin calls register without pluginName?

.register('ctxFromPluginA', context => ..);

now the result is provided to the all plugins, even if they have no dependency on pluginA.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will have to be made impossible by the service owner (which can be a Core service or a plugin).

The service owner that exposes this context container's register method should populate the pluginName argument based on the scoped service contract pattern in #40822

src/core/public/context/context.test.ts Outdated Show resolved Hide resolved
src/core/public/context/context_service.ts Outdated Show resolved Hide resolved
}

describe('ContextContainer', () => {
it('does not allow the same context to be registered twice', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add a test to check an exception, raised during register phase, bubbles to the caller?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean? Do you mean that we move / duplicate this test to ContextService?

};

type ContextServiceContract = PublicMethodsOf<ContextService>;
const createMock = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jest doesn't know which function it mocks. if we want to have type safety, we should provide types manually
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/jest/index.d.ts#L120-L127
There are several options

  • define a type manually for every function
type Arguments<T> = T extends (...args: infer U) => any ? U : never;
//...
const mock = {
    start: jest
      .fn<
        ReturnType<typeof createStartContractMock>,
        Arguments<typeof createStartContractMock>
      >()
      .mockReturnValue(createStartContractMock),
}
  • or define a result type
const mock: jest.Mocked<ContextServiceContract> = {
    start: jest.fn(),
}
// and now, knowing the result type, jest can check whether a passed value satisfies the type
mock.start.mockImplementation(createStartContractMock);

when you declare a type as:

const createMock = (): jest.Mocked<ContextServiceContract> => ({
  setup: jest.fn().mockReturnValue(createSetupContractMock()),
});

TS uses this declaration https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/jest/index.d.ts#L123
and infers a type as jest.fn<any, any>
2019-07-15_13-23-10
Thus, any value passed as mockReturnValue or mockImplementation satisfies this type.

probably there is a better solution I'm not aware of.

rudolf
rudolf previously requested changes Jul 15, 2019
src/core/public/public.api.md Outdated Show resolved Hide resolved
src/core/public/context/context.ts Outdated Show resolved Hide resolved
src/core/public/context/context.ts Outdated Show resolved Hide resolved
src/core/public/context/context.ts Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💔 Build Failed

@joshdover joshdover dismissed rudolf’s stale review July 15, 2019 19:24

Comments addressed

@joshdover
Copy link
Contributor Author

retest

@joshdover
Copy link
Contributor Author

Closing this PR while I experiment with an alternative idea.

@joshdover joshdover closed this Jul 15, 2019
kibana-core [DEPRECATED] automation moved this from Code review to Done (this week) Jul 15, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@joshdover joshdover mentioned this pull request Jul 16, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants