-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Replace event-emitter.js by mitt #5987
Conversation
expect(run).toThrow(/Listener already exists for router event: `sample`/) | ||
}) | ||
|
||
it('should support chaining like the nodejs EventEmitter', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a breaking change? It seems like doing a return this;
in mitt
would be a small change that would avoid surprising those used to node's EventEmitter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid it would make mitt
much bigger in size, because mitt
is not a class but a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't matter; mitt()
returns an object, so this
would refer to that object, and doing a return this
inside of on
and off
would allow for the same type of method chaining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I forgot about that 😅
@timneutkens Should we return this
in on
and off
, ?
It adds ~25 bytes in the minified code (not gzipped).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's fine to add it, however, I'm curious what @developit thinks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mitt doesn't currently return this
because the context of the method isn't guaranteed:
const { emit, on, off } = mitt()
on('foo', console.log)
.emit('foo')
The value of this
when on
is invoked is window/undefined.
This is also true when mixing mitt into an object:
router = Object.assign(Router(), mitt())
router.on() // this is Router instance
If chaining is important as part of the public API here, I think it's best to implement the return semantics here.
This PR aims at replacing next-server/lib/event-emitter.js by mitt.
Fix #4908
event-emitter.js is ~400 bytes gzipped vs mitt is 200 bytes