-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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(microservices): add status, unwrap, on, and other features #14142
feat(microservices): add status, unwrap, on, and other features #14142
Conversation
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.
Looks great! 🙌🏻
// To enable type safety for gRPC. This cant be uncommented by default | ||
// because it would require the user to install the @grpc/grpc-js package even if they dont use gRPC | ||
// Otherwise, TypeScript would fail to compile the code. | ||
// | ||
// type GrpcClient = import('@grpc/grpc-js').Client; |
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.
I wonder if we could add an tsconfig.json#compilerOptions.paths
entry that replaces these optional dependencies with any
on-compile 🤔 You'd get type-safety in the IDE, or when running tsc --noEmit
This would require a separate tsconfigs for development and compilation, and fiddling with the build system is probably not worth the small gain in DX.
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.
tsc
does not auto-replace paths from tsconfig.json#compilerOptions.paths
(it leaves them as is)
Co-authored-by: Rick Dutour Geerling <rick@trilon.io>
Co-authored-by: Rick Dutour Geerling <rick@trilon.io>
} | ||
|
||
public unwrap<T>(): T { | ||
if (!this.mqttClient) { |
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.
Why don't we just call connect() method here directly?
Co-authored-by: Rick Dutour Geerling <rick@trilon.io>
* @param event Event name | ||
* @param callback Callback to be executed when the event is emitted | ||
*/ | ||
public abstract on< |
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.
Hi @kamilmysliwiec,
The change here of adding abstract on
and unwrap
is a breaking change, as now anybody that extended this class must add these methods.
Also, the basic example here is now invalid.
Was this change intentional?
Thanks.
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.
The change here of adding abstract on and unwrap is a breaking change, as now anybody that extended this class must add these methods.
This was released as part of v11 major release - breaking changes are intentional. We'll update the example shortly.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #14044 #11913 #14140
What is the new behavior?
Improvements: graceful reconnection for all transporters, exposed API to listen to internal events, ability to unwrap the client proxy and retrieve the underlying client/server reference, exposed status stream to subscribe to recent client status changes.
Use-cases:
Client:
Client:
Same API for the
ClientProxy
classDoes this PR introduce a breaking change?
Other information