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.

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 SubscribingEventEmitter class #337

Closed
wants to merge 5 commits into from

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Apr 4, 2020

Problem: To register bulk of events, need to invoke ".on("eventname", handler) on each of the events, even handlers are coupled with the class itself, and class knows what events it wants to listen to.

Soliution: Add new class, which extends EventEmitter and adds addSubscriber method.

To make this appear for example to Worker class, it's parent must be updated:

diff --git a/src/core/worker.ts b/src/core/worker.ts
index 6d45c4b..761459a 100644
--- a/src/core/worker.ts
+++ b/src/core/worker.ts
@@ -6,6 +6,7 @@ import { WorkerOptions } from "../types/options";
 import { Connection } from "./connection";
 import { RunPlugins } from "./pluginRunner";
 import { Queue } from "./queue";
+import { SubscribingEventEmitter } from "../utils/SubscribingEventEmitter";

 function prepareJobs(jobs) {
   return Object.keys(jobs).reduce(function (h, k) {
@@ -100,7 +101,7 @@ export type WorkerEvent =
   | "error"
   | "pause";

-export class Worker extends EventEmitter {
+export class Worker extends SubscribingEventEmitter {
   constructor(options, jobs = {}) {
     super();

@glensc
Copy link
Contributor Author

glensc commented Apr 4, 2020

@evantahler let me know what you think of

  • the naming of the class
  • placement of the class in the directory

I will definitely need help on:

  • typing (I used :any and ts-ignore for now)

@glensc
Copy link
Contributor Author

glensc commented Apr 4, 2020

and test cases to cover.

currently, I didn't test how this is passed to event handlers, can do this later if the approach gets a "go", not rejected.

@glensc glensc force-pushed the subscriber-observer branch 2 times, most recently from 569a741 to 74a7392 Compare April 6, 2020 08:33
@glensc
Copy link
Contributor Author

glensc commented Apr 6, 2020

@evantahler added more tests:

  • class method
  • static method of a class

can you help me with typescript types?

@glensc
Copy link
Contributor Author

glensc commented Apr 6, 2020

@evantahler added commit, as seems you don't understand how this all is glued together:

  • 9078f94 Update worker to extend from SubscribingEventEmitter

@glensc
Copy link
Contributor Author

glensc commented Sep 8, 2020

@evantahler you never commented this PR but now just closed?

@evantahler
Copy link
Member

I thought this was coupled to #335? If it's not, can you please provide an example to show how it would be used?

@glensc
Copy link
Contributor Author

glensc commented Sep 9, 2020

@glensc please read the pull request and comments and diff, it's all there including example and tests. and the usage you can read from #335 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants