-
Notifications
You must be signed in to change notification settings - Fork 509
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
Add a CONNECT event to informer. #614
Conversation
src/cache.ts
Outdated
@@ -1,4 +1,4 @@ | |||
import { ADD, DELETE, ERROR, Informer, ListPromise, ObjectCallback, UPDATE } from './informer'; | |||
import { ADD, CONNECT, DELETE, ERROR, Informer, ListPromise, ObjectCallback, UPDATE } from './informer'; |
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.
Does it make sense to add a disconnect here too while we are at it?
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 think connect + error is sufficient. There shouldn't be a disconnect w/o an error that isn't followed immediately by another connect.
src/informer.ts
Outdated
export const ADD: string = 'add'; | ||
export const UPDATE: string = 'update'; | ||
export const DELETE: string = 'delete'; | ||
|
||
// These are issued when a watch connects, reconnects or sees an error |
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 see we are only testing for one of these conditions, what about reconnect or errors?
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.
Added a test for reconnect. There was already a test for errors. I clarified this comment also.
src/cache.ts
Outdated
@@ -97,6 +98,7 @@ export class ListWatch<T extends KubernetesObject> implements ObjectCache<T>, In | |||
// do not auto-restart | |||
return; | |||
} | |||
this.callbackCache[CONNECT].forEach((elt: ObjectCallback<T>) => elt({} as T)); |
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.
Is {} as T
the right thing to do? I would expect no params passed to the callback here (not sure if TypeScript allows expressing that though...)
This also caught me off guard for the ERROR
event - error
is an instance of Error
, not T
.
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.
This was legacy left over from when the callbacks only dealt with objects not errors and connects. I refactored the callback type signature to make it better.
Comments addressed, please re-check. Thanks! |
src/cache.ts
Outdated
@@ -102,13 +113,14 @@ export class ListWatch<T extends KubernetesObject> implements ObjectCache<T>, In | |||
private async doneHandler(err: any): Promise<any> { | |||
this._stop(); | |||
if (err) { | |||
this.callbackCache[ERROR].forEach((elt: ObjectCallback<T>) => elt(err)); | |||
this.callbackCache[ERROR].forEach((elt: ObjectCallback<T>) => elt(undefined, err)); |
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.
Hmmm. This makes it a breaking change? It also has the error at the end, which may be unintuitive to folks used to Node's error-first callbacks... But moving the error first would be even more breaking...
Not sure what's the best approach here... Not saying this shouldn't got ahead, it's easy enough to adapt as a user of the library, but still.
Would be a travesty in TypeScript world to allow on()
to have overloads to accept different parameter types? i.e. allow on(verb: string, fn: ObjectCallback<T>): void;
as well as on(verb: string, fn: ErrorCallback): void;
? While it wouldn't enforce the correct callback type based on the verb
, it would at least be non-breaking? Not sure if there's a better way to express this in TS...
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 did also try it that way:
on(verb: string, fn: ObjectCallback<T> | ErrorCallback | ConnectCallback)
But I felt like that was even messier, because suddenly the user can accidentally pass a ConnectCallback
to a on('add', ...)
Maybe we could do:
export type Callback<T extends KubernetesObject> = (err: any, obj: T | null) => void;
// Deprecated
export type ObjectCallback<T extends KubernetesObject> = (obj: T) => void;
...
on(verb: string, fn: ObjectCallback | Callback)
...
wdyt? That has the benefit of not being a breaking change, and also adding in the more node-style callbacks.
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.
Although, I'm not actually sure that TS can handle that since those two callback types may be indistinguishable...
I made the types more concrete so I think TS can handle it based on the # of parameters.
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.
Would TS handle that at runtime or just compile time?
Did TS ever get enums? If the first param for on()
was enum, rather than a string
, then it could have multiple signatures based on enum type... Which would probably still be a breaking change from TS perspective, but possibly the old string
signature could still be there, just with any
callback so that people can upgrade their code eventually, even if they would lose out a little bit in terms of potentially accidentally passing the wrong callback?
Also I wonder if passing the wrong callback is even a problem for people using TS, because surely someone must have noticed by now that the callback for the error
event is not right? i.e. this could mean that making changes here is not that disruptive, even if the changes are breaking when looking at it strictly?
This is a common pattern (on()
with different callbacks for different event types) - surely someone must have solved this by now? This is used in node itself (in streams and elsewhere): https://github.com/DefinitelyTyped/DefinitelyTyped/blob/1d52dcdeea0c4006bb63c28ff60b919f3f5fc3a6/types/node/stream.d.ts#L84-L91 🤔
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.
was there a follow-up on this @brendandburns ?
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.
Thanks for the pointer! I fixed this using type overloading.
Comments addressed, please take another look! |
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.
👏
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, dominykas The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm Thanks for fixing the comments |
Fixes #609