•  


add push message handler registration and make all pubsub use it. by sjpotter · Pull Request #2735 · redis/node-redis · GitHub
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add push message handler registration and make all pubsub use it. #2735

Draft
wants to merge 4 commits into
base: v5
Choose a base branch
from
Draft

add push message handler registration and make all pubsub use it. #2735

wants to merge 4 commits into from

Conversation

sjpotter
Copy link
Contributor

Description

Describe your pull request here

Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

@sjpotter sjpotter changed the title Push handlers add push message handler registration and make all pubsub use it. Apr 4, 2024
@sjpotter sjpotter mentioned this pull request Apr 6, 2024
Open
3 tasks
leibale
leibale reviewed Apr 8, 2024
View reviewed changes
packages/client/lib/client/commands-queue.ts Outdated
@@ -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>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more .

I'm wondering if we want to have builtInSet or just "hard-code" it instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more .

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)

packages/client/lib/client/commands-queue.ts Outdated
}

addPushHandler(messageType: string, handler: (pushMsg: Array<any>) => unknown) {
if (this.#builtInSet.has(messageType)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more .

Is there a reason to block it? if the user wants a "global" listener - why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more .

this is javascript, dont want to let people hang themselves.

packages/client/lib/client/commands-queue.ts Outdated
}
}

addPushHandler(messageType: string, handler: (pushMsg: Array<any>) => unknown) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more .

Either rename to setPushHandler or have support for multiple listeners, I'm not soo sure which one we wanna go for

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more .

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.

@sjpotter
per github discussion, enable multiple handlers per push message type.
330e266 
discovered that these changes don't make sense for resp2 (no extensible push messages to implement, resp2 implements invaldiate as a pubsub channel)
@sjpotter
Copy link
Contributor Author

redid it to support multiple handlers (can imagine it be useful for debugging).

Sign up for free to join this conversation on GitHub . Already have an account? Sign in to comment
Reviewers

@leibale leibale leibale left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet

2 participants
@sjpotter @leibale
- "漢字路" 한글한자자동변환 서비스는 교육부 고전문헌국역지원사업의 지원으로 구축되었습니다.
- "漢字路" 한글한자자동변환 서비스는 전통문화연구회가 "울산대학교한국어처리연구실 옥철영(IT융합전공)교수팀"에서 개발한 한글한자자동변환기를 바탕하여 지속적으로 공동 연구 개발하고 있는 서비스입니다.
- 현재 고유명사(인명, 지명등)을 비롯한 여러 변환오류가 있으며 이를 해결하고자 많은 연구 개발을 진행하고자 하고 있습니다. 이를 인지하시고 다른 곳에서 인용시 한자 변환 결과를 한번 더 검토하시고 사용해 주시기 바랍니다.
- 변환오류 및 건의,문의사항은 juntong@juntong.or.kr로 메일로 보내주시면 감사하겠습니다. .
Copyright ⓒ 2020 By '전통문화연구회(傳統文化硏究會)' All Rights reserved.
 한국   대만   중국   일본 

Close