-
Notifications
You must be signed in to change notification settings - Fork 11
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(repeater): refrain from utilizing non-standard ports #404
Conversation
fce13e5
to
476bd79
Compare
92e4a62
to
98dbdad
Compare
d740100
to
9f1a70d
Compare
67eff22
to
78ea71e
Compare
src/Repeater/RepeaterServer.ts
Outdated
export type RepeaterServerEventHandler<P, R = void> = P extends undefined | ||
? () => R | Promise<R> | ||
: (payload: P) => R | Promise<R>; |
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 event listeners have nothing in common with each other. Let's inline their types to make it more cohesive.
Additionally, if you're not interested in the return value, declare the return type as unknown
, this is the more user-friendly approach. Also, get rid of redundant wrappers, such RepeaterServerReconnectionFailedEvent
For details please refer to the example below:
requestReceived(
handler: (request: RepeaterServerRequestEvent) => RepeaterServerRequestResponse | Promise< RepeaterServerRequestResponse >
): void;
reconnectionFailed(
handler: (error: Error) => unknown
): void;
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.
Done.
But I preserved void
type for handlers which are not supposed to return anything. It seems to be more clear from handler's contract perspective. If I used unknown
it would mean that handlers expect some data to be returned, that further will be processed. But with void
it clearly says that no data is needed to be returned
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.
If I used unknown it would mean that handlers expect some data to be returned, that further will be processed.
You already handle the never type silently, hiding these details from the public API by using the void
type.
Avoid using the void
return type for callbacks when the return value will be ignored or handled (e.g. the never
type). Instead, use unknown
to explain clearly that you are not interested in the return value, regardless of what it might be, and gracefully handle all cases.
Anyway, you can keep it as for now, since this is part of internal API.
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.
Avoid using the void return type for callbacks when the return value will be ignored or handled (e.g. the never type). Instead, use unknown to explain clearly that you are not interested in the return value, regardless of what it might be, and gracefully handle all cases.
I think I don't fully understand what benefits unknown
gives us in such cases.
unknown
exactly says that i'm interested in returned value, but not in it's type. void
says that it doesn't expect any value to be returned, so it won't be processed even if returned
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.
, so it won't be processed even if returned
However, you do. You silently handle the never, is not it so?
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.
Actually yes, but that's an implementation detail which in combination with void
type will not happen
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.
These are not implementation details. Such "details" represent the post- and pre-conditions of any interface. Using the void
here could potentially result in incorrect inheritance, contract and LSP violations.
BREAKING CHANGE: Remote repeater scripts are deprecated and no longer supported. Instead, please utilize the local scripts with the following command:
closes #419