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

[Implement] Naive basic EventEmitter class #18

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

jtenner
Copy link
Contributor

@jtenner jtenner commented Jul 25, 2019

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.

@jtenner jtenner added the enhancement New feature or request label Jul 25, 2019
@jtenner jtenner requested a review from dcodeIO July 25, 2019 20:44
@jtenner jtenner self-assigned this Jul 25, 2019
@jtenner
Copy link
Contributor Author

jtenner commented Jul 25, 2019

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
Contributor Author

jtenner commented Aug 13, 2019

@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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant