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

Typings fix and improvements. #1364

Merged
merged 10 commits into from
May 31, 2019
Merged

Typings fix and improvements. #1364

merged 10 commits into from
May 31, 2019

Conversation

deskoh
Copy link
Contributor

@deskoh deskoh commented May 23, 2019

  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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants