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

Regresssion in PR#96 #97

Closed
thetutlage opened this issue May 17, 2022 · 4 comments · Fixed by #98
Closed

Regresssion in PR#96 #97

thetutlage opened this issue May 17, 2022 · 4 comments · Fixed by #98

Comments

@thetutlage
Copy link

The #96 introduces a regression when emittery is used as a sub-class. Lemme share a code sample.

const Emittery = require("emittery");

class MyEmitter extends Emittery {
  async emit(eventName, data) {
  	return super.emit(eventName, data)
  }
}

new MyEmitter().on('foo', () => {})

Adding a listener foo will raise an exception from this line https://github.com/sindresorhus/emittery/blob/main/index.js#L262, since it is calling this.emit with three arguments.

Now, I can update the emit method on my class to accept the 3rd argument and pass it to super.emit. However, then TypeScript complains that the emit method takes only two arguments.

thetutlage added a commit to japa/core that referenced this issue May 17, 2022
@sindresorhus
Copy link
Owner

// @lukehorvat

@lukehorvat
Copy link
Contributor

Ouch. Sorry for the oversight.

I have one idea. We could have a private withMetaEventsAllowed function which would allow Emittery to run some internal code in a "privileged" way (like running sudo or whatever). All of the internal calls to emit could be wrapped in this function.

let metaEventsAllowed = false;

function withMetaEventsAllowed(callback) {
  metaEventsAllowed = true;
  callback();
  metaEventsAllowed = false;
}

So, for example, this code:

emittery/index.js

Lines 261 to 263 in f61b87d

if (!isMetaEvent(eventName)) {
this.emit(listenerAdded, {eventName, listener}, metaEventsAllowed);
}

Would become this:

if (!isMetaEvent(eventName)) {
  withMetaEventsAllowed(() => {
    this.emit(listenerAdded, {eventName, listener});
  });
}

This would eliminate the need for emit to have a third param and subclasses could do whatever they want. Of course, the downside is that it introduces some global state (but since it wouldn't be exposed to userland maybe it's not so bad?).

Just a rough idea I had; could probably be improved. But if it sounds fine to you then I'll go ahead and make a PR.

@thetutlage
Copy link
Author

Yup. Or we can have a special function that emits the meta events on a given emitter instance. Something like this.

async function emitMetaEvent(emitter, event, data) {
  metaEventsAllowed = true
  await emitter.emit(event, data)
  metaEventsAllowed = false
}

The only benefit of this function over the callback approach is, we will not end up creating too many anonymous functions.

if (!isMetaEvent(eventName)) {
  emitMetaEvent(this, listenerAdded, {eventName, listener})
}

@sindresorhus
Copy link
Owner

Or we can have a special function that emits the meta events on a given emitter instance. Something like this.

👍 Sounds good

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 a pull request may close this issue.

3 participants