Conversation
💔 Build Failed |
99b845c to
d4b3ca2
Compare
💚 Build Succeeded |
d4b3ca2 to
3172234
Compare
💔 Build Failed |
3172234 to
06f5e77
Compare
|
Pinging @elastic/kibana-platform |
06f5e77 to
d496577
Compare
💚 Build Succeeded |
d496577 to
68a7501
Compare
💚 Build Succeeded |
eliperelman
left a comment
There was a problem hiding this comment.
Looks really good! Left a couple small nits.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think @restrry knows more. I believe it's a fixable problem in DefinitelyTyped but not sure if it's been fixed yet?
There was a problem hiding this comment.
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>

Thus, any value passed as mockReturnValue or mockImplementation satisfies this type.
probably there is a better solution I'm not aware of.
68a7501 to
3f098b2
Compare
💚 Build Succeeded |
src/core/public/context/context.ts
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
docs/development/core/public/kibana-plugin-public.contextcontainer.register.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
should we add a test to check an exception, raised during register phase, bubbles to the caller?
There was a problem hiding this comment.
I'm not sure what you mean? Do you mean that we move / duplicate this test to ContextService?
There was a problem hiding this comment.
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>

Thus, any value passed as mockReturnValue or mockImplementation satisfies this type.
probably there is a better solution I'm not aware of.
3f098b2 to
2871cfc
Compare
2871cfc to
4306f77
Compare
💔 Build Failed |
|
retest |
|
Closing this PR while I experiment with an alternative idea. |
💔 Build Failed |
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:
To do in future PRs:
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers