Skip to content

[Implement] Naive basic EventEmitter class - #18

Open
jtenner wants to merge 5 commits into
mainfrom
event-emitter
Open

[Implement] Naive basic EventEmitter class#18
jtenner wants to merge 5 commits into
mainfrom
event-emitter

Conversation

@jtenner

@jtennerjtenner commented Jul 25, 2019

Copy link
Copy Markdown
Contributor

Let me start by saying I realize how entirely dangerous this is. Currently there's no good ergonomic way to do this, and in order to implement the backbone of the Stream classes, we need something that resembles an emitter.

Current Limitations:

  1. Callback signatures must match runtime assertions
  2. EventEmitter is treated as abstract
  3. Callback parameters up to length 10
  4. callback must return void
  5. Emit still too large. Requires switch statement to determine callback argument length
  6. emit function doesn't match @types/node type arguments. It still requires explicit types
emitter.emit<A,B,C,D>(a: A,b: B,c: C,d: D);// etc.

@jtennerjtenner added the enhancement New feature or request label Jul 25, 2019
@jtenner
jtenner requested a review from dcodeIOJuly 25, 2019 20:44
@jtennerjtenner self-assigned this Jul 25, 2019
@jtenner

jtenner commented Jul 25, 2019

Copy link
Copy Markdown
ContributorAuthor

Now the implementation contains a groked callback type verifier that does runtime checking of callback signatures. It requires a global map that contains a set of valid signatures for each event, and is registered once per EventEmitter class extension.

Not done:

  • EventEmitter should be abstract.
  • Docs needs to be updated for this class so that others can learn how to use this class effectively
  • Wait for compiler to support variable argument lengths or inferring of argument length count
    • will be a 64bit integer with both the id and the argument length combined

@jtenner

Copy link
Copy Markdown
ContributorAuthor

@dcodeIO conflicts aside, what's holding this request back? I just need to fix the builtin request. Right?

Other than that, not sure how to make this better.

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

Labels

enhancementNew feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@jtenner