Add context type parameter to IRouter and createRouter
#57674
Conversation
pgayvallet
added
Team:Core
|
Pinging @elastic/kibana-platform (Team:Platform) |
src/core/server/http/types.ts
Outdated
| createRouter: <Context extends RequestHandlerContext = RequestHandlerContext>( | ||
| path: string, | ||
| plugin?: PluginOpaqueId | ||
| ) => IRouter<Context>; | ||
| getAuthHeaders: GetAuthHeaders; | ||
| registerRouteHandlerContext: <T extends keyof RequestHandlerContext>( | ||
| pluginOpaqueId: PluginOpaqueId, |
There was a problem hiding this comment.
Now that we are more or less deprecating the
declare module 'src/core/server' {
interface RouteHandlerContext {
myField: MyType;
}
}approach in favor of that, I think the
registerRouteHandlerContext: <T extends keyof RequestHandlerContext>(
contextName: T,
provider: RequestHandlerContextProvider<T>
) => RequestHandlerContextContainer;should change to something like
registerRouteHandlerContext: <T extends string>(
contextName: T,
provider: RequestHandlerContextProvider<T>
) => RequestHandlerContextContainer;Else, plugin would still need to use the declare statement to be able to add the key to the RequestHandlerContext before registering. WDYT?
There was a problem hiding this comment.
Only other option I can is making the context type here generic as well, so it'd be:
registerRouteHandlerContext: <
TContext extends RequestHandlerContext,
T extends keyof TContext
>(
contextName: T,
// this would probably need to be changed to accept a type arg as well
// ...and possibly IContextContainer would need to be changed?
provider: RequestHandlerContextProvider<T>
) => RequestHandlerContextContainer;Only concern with this is that the generics in IContextContainer are already confusing...adding more generics might make this worse?
There was a problem hiding this comment.
I think we have enough generics in this part of the code yes and would like to avoid introducing new confusing ones yes. Also with your proposition, I think the call to registerRouteHandlerContext will always to specify the TContext type as it cannot be guessed from the call:
http.registerRouteHandlerContext<MyCustomRequestHandlerContext>('myKey', provider)Although, my example is also false:
registerRouteHandlerContext: <T extends string>(
contextName: T,
provider: RequestHandlerContextProvider<T>
) => RequestHandlerContextContainer;Still doesn't work without the declare statement, as the provider is still expected to be a RequestHandlerContextProvider<T>, which is:
export type RequestHandlerContextProvider<
TContextName extends keyof RequestHandlerContext
> = IContextProvider<RequestHandler<any, any, any>, TContextName>;which is(...)
export type IContextProvider<
THandler extends HandlerFunction<any>,
TContextName extends keyof HandlerContextType<THandler>
> = (
context: Partial<HandlerContextType<THandler>>,
...rest: HandlerParameters<THandler>
) =>
| Promise<HandlerContextType<THandler>[TContextName]>
| HandlerContextType<THandler>[TContextName];Meaning it still expects to resolve it's type from a property of RequestHandlerContext.
Changing the registerRouteHandlerContext signature is going to be way harder than anticipated. Really not sure of the direction to take, the context types in src/core/utils/context.ts are really hard to follow... My guess is that we should cut the relation between the context name and the context provider when registering, as there is no longer a direct link between the name of the context and the provider's result type.
However that's some big changes as it means changing the context base types, and these are used elsewhere. So I'm starting to think your proposition is the only realistic one...
WDYT?
There was a problem hiding this comment.
My guess is that we should cut the relation between the context name and the context provider when registering, as there is no longer a direct link between the name of the context and the provider's result type.
I personally prefer that we maintain type safety here so that we can be sure that the provider that is registered is actually returning what it is supposed to.
However, I agree the generics in src/core/utils/context.ts are already confusing. One option could be to only add the generics to registerRouteHandlerContext and cast it to any when passing it over to IContextContainer#registerContext. Reason I think this may be acceptable at this time is that we may be able to replace the super generic IContextContainer with a concrete implementation just for route handlers once we remove ApplicationService's context completely (already deprecated).
There was a problem hiding this comment.
@joshdover I implemented what we discussed about yesterday on slack. PTAL, would like a second opinion on whereas this is still worth the change.
| registerRouteHandlerContext: <T extends keyof RequestHandlerContext>( | ||
| contextName: T, | ||
| provider: RequestHandlerContextProvider<T> | ||
| registerRouteHandlerContext: < | ||
| TContextType extends RequestHandlerContext = RequestHandlerContext, | ||
| TContextName extends keyof TContextType = 'core' | ||
| >( | ||
| contextName: TContextName, | ||
| provider: RequestHandlerContextProvider<TContextName, TContextType> |
There was a problem hiding this comment.
So, some explanations on that:
I introduced the new TContextType parametrized type. this one has a default, but is explicit. TContextName had no default, but was explicit. as defaulting generics must be declared after non-default ones, I could have done
registerRouteHandlerContext: <
TContextName extends keyof TContextType
TContextType extends RequestHandlerContext = RequestHandlerContext,
>However, the TContextName is inferred from the input parameter, and TContextType is not. That would have forced to declare the name generic on every calls:
http.registerRouteHandlerContext<'search', MyCustomHandlerContext>('search', provider)The default=core trick allow to reverse the generics order, to have the inferred one last, and allow not to declare it:
http.registerRouteHandlerContext<MyCustomHandlerContext>('search', provider)| registerRouteHandlerContext: setupDeps.core.http.registerRouteHandlerContext.bind( | ||
| null, | ||
| this.legacyId | ||
| ), | ||
| createRouter: () => setupDeps.core.http.createRouter('', this.legacyId), | ||
| registerRouteHandlerContext: < | ||
| TContextType extends RequestHandlerContext = RequestHandlerContext, | ||
| TContextName extends keyof TContextType = 'core' | ||
| >( | ||
| contextName: TContextName, | ||
| provider: RequestHandlerContextProvider<TContextName, TContextType> | ||
| ) => setupDeps.core.http.registerRouteHandlerContext(this.legacyId, contextName, provider), |
There was a problem hiding this comment.
With the generic changes, I did not find a way to make TS happy about the bind call. I was forced to reproduce manually the registerRouteHandlerContext signature. See next comment for root cause.
| registerRouteHandlerContext: jest.fn(), | ||
| registerRouteHandlerContext: jest.fn() as any, |
There was a problem hiding this comment.
This is where it become very ugly, and may force us to reconsider the whole PR:
The introduced generic 'breaks' the jest mock, as it makes some parameters non-inferrable:
Error:(104, 5) TS2322: Type 'Mock<any, any>' is not assignable to type 'MockInstance<RequestHandlerContextContainer, never> & (<TContextType extends RequestHandlerContext = RequestHandlerContext, TContextName extends keyof TContextType = "core">(contextName: TContextName, provider: IContextProvider<...>) => RequestHandlerContextContainer)'.
Type 'Mock<any, any>' is not assignable to type 'MockInstance<RequestHandlerContextContainer, never>'.
Types of property 'mock' are incompatible.
Type 'MockContext<any, any>' is not assignable to type 'MockContext<RequestHandlerContextContainer, never>'.
Type 'any' is not assignable to type 'never'.This is caused by the (only possible) way jest retrieve the function parameters:
type Mocked<T> = {
[P in keyof T]: T[P] extends (...args: any[]) => any
? MockInstance<ReturnType<T[P]>, ArgsType<T[P]>>
: T[P] extends Constructable
? MockedClass<T[P]>
: T[P]
} &
T;
type ArgsType<T> = T extends (...args: infer A) => any ? A : never;But the parameters are no longer inferrable, probably due to the fact that the RequestHandlerContext is now generified. The effective type of the mock becomes MockInstance<RequestHandlerContextContainer, never>.
The root cause seems to be the type inferring we are doing for the HandlerContextType, which makes TS no longer able to infer the upper inferring when resolving jest.mock
export type HandlerContextType<T extends HandlerFunction<any>> = T extends HandlerFunction<infer U>
? U
: never;This forces to cast the jest.fn to any, which is kinda ugly. But I don't think we really have any other easy solution...
|
Should we close this PR? I know there are still some cleanup tasks, but this seems more or less stale at this point. |
💔 Build FailedTest FailuresX-Pack Jest Tests.x-pack/legacy/plugins/uptime/public/components/functional/monitor_list/__tests__.MonitorList component renders the monitor listStandard OutStack TraceX-Pack Jest Tests.x-pack/legacy/plugins/uptime/public/components/functional/charts/__tests__.PingHistogram component renders the component without errorsStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
Summary
Fix #57588
Adds a
Contexttype to theIRouterinterface and concrete type to specify the type of theRequestHandlerContextexpected to be accessible from the routes created with the router.Documentation has not been updated yet. This is a POC to validate that this is the approach we want instead of the
declarestatement thing.Checklist
For maintainers