feat: application service types default to any#1566
Conversation
11f709e to
e12e505
Compare
e12e505 to
5c666c5
Compare
|
This will enforce compulsory typings for all service. Possible to have best of both worlds? |
|
This will not enforce compulsory typings. Here is the code which you can check in typescript playground: interface ApplicationOld< ServiceTypes = {} > {
service< L extends keyof ServiceTypes >( location: L ): ServiceTypes[ L ];
service( location: string ): keyof ServiceTypes extends never ? any : never;
}
interface ApplicationNew< ServiceTypes = any > {
service< L extends keyof ServiceTypes >( location: L ): ServiceTypes[ L ];
}
const oldUnspecified = undefined as any as ApplicationOld;
const oldSpecified = undefined as any as ApplicationOld< { foo: 'bar' } >;
const newUnspecified = undefined as any as ApplicationNew;
const newSpecified = undefined as any as ApplicationNew< { foo: 'bar' } >;
const oldUnFoo = oldUnspecified.service('foo');
const oldUnBaz = oldUnspecified.service('baz');
const oldSeFoo = oldSpecified.service('foo');
const oldSeBaz = oldSpecified.service('baz'); // never
const oldConvert: ApplicationOld<{ foo: 'bar' }> = oldUnspecified; // error;
const newUnFoo = newUnspecified.service('foo');
const newUnBaz = newUnspecified.service('baz');
const newSeFoo = newSpecified.service('foo');
const newSeBaz = newSpecified.service('baz'); // type cheching error
const newConvert: ApplicationNew<{ foo: 'bar' }> = oldUnspecified; |
|
Ah. I get it now. |
|
Was this closed accidentally too? |
|
Dang, yes, sorry. This should also stay open to be either merged or revisited for v5. |
5c666c5 to
1da5a09
Compare
|
Rebased and adapted. |
|
Great, thank you! Will have a look shortly but if it doesn't break any of the latest tests this should work well. |
|
Perfect! This totally should fix some of the problems I was having before, too. Will go out with the beta release shortly. |
|
Hm, strange, looked like the build passed but is failing once it was merged. |
|
Any particular problem? |
|
Type incompatibility: https://github.com/feathersjs/feathers/runs/2156522850?check_suite_focus=true#step:6:235 Getting the errors locally with the latest |
|
I have a feeling this might be something else. The compilation error I'm getting locally has something to do with the grant package. |
|
Other problem fixed for now and I am getting the same error: Problem seems to be that the Express app type is no longer compatible with the basic Feathers app type (apparently it was before for some reason). I tried updating interface ExpressUseHandler<T, ServiceTypes> {
<L extends keyof ServiceTypes & string> (
path: L,
...middlewareOrService: (
Express|express.RequestHandler|
(keyof any extends keyof ServiceTypes ? ServiceInterface<any> : ServiceTypes[L])
)[]
): T;
(...expressHandlers: express.RequestHandler[]): T;
(handler: Express|express.ErrorRequestHandler): T;
}
export interface Application<ServiceTypes = any, AppSettings = any>
extends FeathersApplication<ServiceTypes, AppSettings>, Omit<Express, 'listen'|(keyof FeathersApplication)> {
listen(port: number, hostname: string, backlog: number, callback?: () => void): Promise<http.Server>;
listen(port: number, hostname: string, callback?: () => void): Promise<http.Server>;
listen(port: number|string|any, callback?: () => void): Promise<http.Server>;
listen(callback?: () => void): Promise<http.Server>;
use: ExpressUseHandler<this, ServiceTypes>;
}But no luck so far. |
In #1364
ServiceTypesdefault changed fromanyto{}with an intention to makeapp.service()act more strict whenServiceTypesis provided. Desired outcome and expected type resolution (specified in second comment from that PR) were right, but the solution had a flaw.When you make
ServiceTypesdefault to{},Applicationwithout parameters (like inHookContext) becomesApplication< {} >. And you can't assignApplication< {} >toApplication< MyServiceTypes >:As shown
Application< any >does not have such drawback.To achieve desired outcome right solution is to simply remove second catch-all
service()overload, because whenServiceTypesisanyfirst definition is already catch-all and whenServiceTypesis notanythen first definition acts as a strict one.There is a little breaking change (that why it is not a fix, but a feat) since now
{}stops acting as wildcard and not a default value.