E52B Typings fix and improvements. by deskoh · Pull Request #1364 · feathersjs/feathers · GitHub
[go: up one dir, main page]
Skip to content

Typings fix and improvements.#1364

Merged
daffl merged 10 commits intofeathersjs:masterfrom
deskoh:master
May 31, 2019
Merged

Typings fix and improvements.#1364
daffl merged 10 commits intofeathersjs:masterfrom
deskoh:master

Conversation

@deskoh
Copy link
Contributor
@deskoh deskoh commented May 23, 2019 •
edited
Loading
  1. Fixed typings for Service.
- service<L extends keyof ServiceTypes> (location: L): ServiceTypes[L];
+ service<L extends keyof ServiceTypes> (location: L): Service<ServiceTypes[L]>;
  1. Improvement in service typings is described as follows.

Current typings consists of a catch-all for any service name:

export interface Application<ServiceTypes = any> extends EventEmitter {

    service<L extends keyof ServiceTypes> (location: L): Service<ServiceTypes[L]>;

    // vvv: Catch-all for any service name
    service (location: string): Service<any>;
}

This allows service type to resolve if no ServiceType is specified.

const app = feathersExpress(feathers());
let service1 = app.service('messages'); // service1: Service<any>

However, if ServiceType is specified, invalid service name should not be resolved:

type Message = { text: string };
type App = Application<{ messages: Message }>;

const app2 = feathersExpress(feathers()) as App;
let service3 = app2.service('messages'); // service3: Service<Message>
let service4 = app2.service('message');  // service4: never <- Will resolve to Service<any> for current typings

Defaulting the ServiceType to {} and modifying the 'catch-all' typings can fulfill the above:

export interface Application<ServiceTypes = {}> extends EventEmitter {

    service<L extends keyof ServiceTypes> (location: L): Service<ServiceTypes[L]>;

    // vvv: Modified catch-all for service name if `ServiceType` is not provided
    service (location: string): keyof ServiceTypes extends never ? any : never;
}

@deskoh
Copy link
Contributor Author
deskoh commented May 23, 2019

Expected type resolution:

let app: Application<{ users: User }>;
app.service('users'); // Service<User>
app.service('user');  // never

let app2: Application<any>;
app2.service('users'); // Service<any>
app2.service('user');  // Service<any>

let app3 = Application; // equivalent to Application<never>
app3.service('users'); // Service<any>
app3.service('user');  // Service<any>

@deskoh
Copy link
Contributor Author
deskoh commented May 23, 2019

Test seems to fail due to the fix below:

-        service<L extends keyof ServiceTypes> (location: L): ServiceTypes[L];
+        service<L extends keyof ServiceTypes> (location: L): Service<ServiceTypes[L]>;

@daffl
Copy link
Member
daffl commented May 31, 2019

Thank you for the pull request. If someone just uses const app: Application without specifying the services mappings, it would still work (it'll just be returned as any) right?

@deskoh
Copy link
Contributor Author
deskoh commented May 31, 2019

Yeap.

@daffl
Copy link
Member
daffl commented May 31, 2019

Great. Thanks again!

@daffl daffl merged commit 515b916 into feathersjs:master May 31, 2019
@deskoh deskoh mentioned this pull request Jun 13, 2019
Closed
@vonagam vonagam mentioned this pull request Sep 19, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@deskoh @daffl
0