add push message handler registration and make all pubsub use it. #2735
Conversation
sjpotter
changed the title
| @@ -51,6 +53,8 @@ export default class RedisCommandsQueue { | |||
| #chainInExecution: symbol | undefined; | |||
| readonly decoder; | |||
| readonly #pubSub = new PubSub(); | |||
| readonly #pushHandlers: Map<string, (pushMsg: Array<any>) => unknown> = new Map(); | |||
| readonly #builtInSet: ReadonlySet<string>; | |||
There was a problem hiding this comment.
I'm wondering if we want to have builtInSet or just "hard-code" it instead?
There was a problem hiding this comment.
i think the set object would remain, much quicker to lookup in a set than multiple if conditions. not that this is very commonly used code path (i.e. just at registration/unregistration time)
| } | ||
|
|
||
| addPushHandler(messageType: string, handler: (pushMsg: Array<any>) => unknown) { | ||
| if (this.#builtInSet.has(messageType)) { |
There was a problem hiding this comment.
Is there a reason to block it? if the user wants a "global" listener - why not?
There was a problem hiding this comment.
this is javascript, dont want to let people hang themselves.
| } | ||
| } | ||
|
|
||
| addPushHandler(messageType: string, handler: (pushMsg: Array<any>) => unknown) { |
There was a problem hiding this comment.
Either rename to setPushHandler or have support for multiple listeners, I'm not soo sure which one we wanna go for
There was a problem hiding this comment.
hmm. It's an interesting Q (and then how would remove work?) (and would fit with allowing it to exist with current handlers). Let me think.
discovered that these changes don't make sense for resp2 (no extensible push messages to implement, resp2 implements invaldiate as a pubsub channel)
|
redid it to support multiple handlers (can imagine it be useful for debugging). |
Description
Checklist
npm testpass with this change (including linting)?