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

RFC: add support for Service Request and Response type #2125

Closed
wants to merge 1 commit into from

Conversation

bwgjoseph
Copy link
Contributor

Hi,

This PR would likely be a breaking change, so I am actually hoping to make it into dove instead of crow. I have most of the description written in this-repo that I created to showcase, but I will put some of key info here.


For the longest time, I have been wanting to have some ways to allow me to define a different set of request/response interface since it can be different (and it is for most cases that I have encounter). There have been some chats about this in slack as well, and I thought I can start making some changes and see if this can be accepted, and I'm sure can be further improved as well.

https://feathersjs.slack.com/archives/C52QSFK0A/p1594138177347400
https://feathersjs.slack.com/archives/C3M4SBWT0/p1600014049010600

Objective

Allow to declare a different set of interface for Request and Response which can also greatly improve dev-experience.

Example

// post.interface.ts
interface Base {
    readonly _id: string;
    createdAt: Date;
    updatedAt: Date;
}

/ Client facing interface
interface PostRequest {
    title: string;
    body: string;
    comments?: string[];
}

// Base fields generated at server side
interface ServerPostRequest extends PostRequest, Omit<Base, '_id'> {}

interface PostResponse extends Omit<PostRequest, 'comments'>, Base {}

// post.service.ts
declare module '../../declarations' {
  interface ServiceTypes {
    'post': Service<PostRequest, PostResponse> & Post;
  }
}

// if you choose not to have different request/response interface, you can still do this
// the response type will still be PostInterface
declare module '../../declarations' {
  interface ServiceTypes {
    'post': Service<PostInterface> & Post;
  }
}

Result

infer-create-request-1
infer-create-request-2
infer-create-response-1
infer-create-response-2


For more details, and example, please refer to feathers-request-response-demo

If there is a better way to do this, I would like to have your feedback. This should not affect any actual functionality of the application, it all just providing better typing's for typescript developers.

Thanks!

@bwgjoseph
Copy link
Contributor Author

@daffl would this be considered for v5 seeing that the first pre-release is out. Thanks

@daffl
Copy link
Member

daffl commented Mar 19, 2021

This is a good suggestion. Since there already was a major refactoring in the works (PR via #2255) I included it there so we'll close this PR. Thank you for putting this up!

@daffl daffl closed this in #2255 Mar 19, 2021
@bwgjoseph
Copy link
Contributor Author

Thanks @daffl!

Will try it out after you have release a alpha/beta for it. The only thing that I noticed (now, with a quick look) is that you had the request at the 2nd arg, and response at the first. ServiceInterface<Response, Request = Response>. I guess it make sense if majority of the usage is without a different request, then there is no need to define the 2nd arg. 👍

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.

2 participants