-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[usage-api] Use better typescript generator #12903
Conversation
5515055
to
4516a51
Compare
4516a51
to
fb84cd7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR could benefit from a nicer description to explain that it's using nice-grpc which is a higher level abstraction on top of improbable/grpc-web and uses modern TS to wrap the client into a more usable form.
Change looks great, thanks for digging into this. Let's see how it works for the usage service and we can then migrate the remaining grpc clients instances.
@@ -264,19 +258,14 @@ export const productionContainerModule = new ContainerModule((bind, unbind, isBo | |||
|
|||
bind(NewsletterSubscriptionController).toSelf().inSingletonScope(); | |||
|
|||
bind(UsageServiceClientConfig).toDynamicValue((ctx) => { | |||
bind<UsageServiceClient>(UsageServiceDefinition.name).toDynamicValue((ctx) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svenefftinge, shouldn't that be bound in singleton scope?
Asking, because of that one of the changes being introduced/deployed after which we see a severe memory leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it can be in singleton scope as well. But if this increases leakage it could be there are other components that leak as well and hold references to these instances.
Description
This PR uses an alternative typescript generator for the protobuf bindings, that generates more ideomatic TypeScript.
Related Issue(s)
Fixes #12880
How to test
Release Notes
Documentation
Werft options: