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

feat: add before and after request methods to base service #961

Merged

Conversation

lukealvoeiro
Copy link
Collaborator

@lukealvoeiro lukealvoeiro commented Nov 3, 2023

Description

Consumers of any generated service may want to abstract away functionality like additional error parsing or request/response logging into their Rpc implementation, as this lets them perform this shared logic across all requests and responses they have. This PR implements this change, adding two settings to ts_proto_opt:

  • outputBeforeRequest: boolean (defaults to false)
  • outputAfterResponse: boolean (defaults to false)

When true, these will allow you to define an Rpc with two additional, optional methods - beforeRequest and afterResponse

Testing

  • Test conversion has been added to the integration folder
  • Jest tests have been set up to confirm beforeRequest and afterResponse are only called when they are set.

Documentation

  • Have updated main README.md to include the two new settings

Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @lukealvoeiro , I had a few questions, but this is a great PR, thanks for submitting it!

integration/before-after-request/simple.ts Outdated Show resolved Hide resolved
src/options.ts Outdated Show resolved Hide resolved
const additionalMethods = [];
const is$TypePresent = options.outputTypeAnnotations || options.outputTypeRegistry;
if (options.outputAfterResponse) {
additionalMethods.push(code`afterResponse?(response: { ${is$TypePresent ? "$type: string" : ""} }): void;`);
Copy link
Collaborator Author

@lukealvoeiro lukealvoeiro Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephenh Since the request / response type is truly dynamic, I was hoping to get your thoughts on this call signature. Options I can think of are the following:

// OPTION 1: (Current)
beforeRequest: (request: {}): void; // or
beforeRequest: (request: { $type: string }): void;

// OPTION 2: using a index signature
beforeRequest: (request: {[property: string]: any)}: void;

// OPTION 3: using any or object 😢 
beforeRequest: (request: object): void;

Note that I would be adding an inline comment in ts-proto and to the generated file to point this out.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yeah, I dunno, isn't request: object kinda normal for these middleware-esque patterns?

I.e. if you want to listen to "any message going in/out", there really isn't a common/base interface for all messages.

To your point, unless the user has the type registry option turned on, then they'll at least have the $type key and can do some "if message.$type === ..." checks.

So maybe request: object by default, by request: { $type: string } if the type registry feature is enabled?

Fwiw I'm not super-close to the nuances of object vs. the index signature, so if you've done some WIP coding of these handlers, and the index signature makes the handler code more ergonomic, I'm good with that too!

Copy link
Collaborator Author

@lukealvoeiro lukealvoeiro Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ended up going with the following, as after looking up object I realized its still a bit of a wide net:

afterResponse?<T extends { [k in keyof T]: unknown }>(response: T): void;

@stephenh
Copy link
Owner

stephenh commented Nov 9, 2023

Cool, lgtm, thanks @lukealvoeiro !

@stephenh stephenh merged commit 19ba6a5 into stephenh:main Nov 9, 2023
6 checks passed
stephenh pushed a commit that referenced this pull request Nov 9, 2023
# [1.164.0](v1.163.0...v1.164.0) (2023-11-09)

### Features

* add before and after request methods to base service ([#961](#961)) ([19ba6a5](19ba6a5))
@stephenh
Copy link
Owner

stephenh commented Nov 9, 2023

🎉 This PR is included in version 1.164.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants