T/144: Refactor EmitterMixin methods.

[jodator]

Suggested merge commit message (convention)

Other: Aligned behaviors of EmitterMixin methods responsible for adding end removing listeners. Closes ckeditor/ckeditor5#4947.

The emitter.on now has the same behavior as emitter.listenTo( emitter ) as well as emitter.off is the same as emitter.stopListening( emitter ).

---

Additional information

• Requires Other: Remove context options form EmitterMixin. #189 to be closed first as this branch is based on previous work.
• This PR clean up on/listenTo and off/stopListening so the on/off are purely shorthands for base methods.
• I had to make minor changes to internal Proxy in DomEmitterMixin as it used listenTo/on in pretty convoluted way. I've renamed Proxy methods to not confuse them with EmitterMixin ones.
• Only changes besides EmitterMixin that were required were in DomEmitterMixin.

[jodator]

I've found one bug in code. But probably it will be better to handle it with #191.

The self.stopListening( emitter, 'foo', cb ); does not remove listeningTo entry from self.

[oleq]

If you used listenTo( emitter, ...rest ) here, you'd be able to use proxy.attach( ...rest ); and EmitterMixin.listenTo.call( this, emitter, ...rest ); later in the code. IMO, if the method does not process these arguments and just passes them through, it's safer to use rest.

[oleq]

Never heard of the guy 😛

[jodator]

I've rebased this branch and "fixed" coverage - I cannot test that check but I feel uncomfortable with just removing it so I've ignored it for coverage.

[Reinmar]

AFAICS, event can be undefined. Which means calling emitter.detach() but its first param is not optional. Could you verify that it all works fine in cases where stopListening()'s params are not passed?

[jodator]

You mean besides tests? The event parameter is check by the EmitterMixin.stopListening() and also in here dom/EmitterMixinProxy.detach(): https://github.com/ckeditor/ckeditor5-utils/pull/190/files/86c8efc9cff0415a01cdd3cd2a29aef81e1aadce#diff-99d3cd5d9460b6839252663e7c38b1c6R218 - actually it is checked if given event is defined in _domListeners so if it is falsy then detach will do nothing.

[Reinmar]

It's handled very poorly there, because if event is undefined then we do listeners[ undefined ]. And the API docs of detach() are also incorrect.

[Reinmar]

In other words – if we expect event to be undefined then it should be handled explicitly. However, it's not the problem of your PR – it was like that in the past.

[Reinmar]

Actually, it seems to be a bug: https://github.com/ckeditor/ckeditor5-utils/issues/199

[Reinmar]

OK, unfortunately, this is an R- because of lack of tests. A change must always be accompanied by tests unless this is purely internal refactoring.

[Reinmar]

I've rebased this branch and "fixed" coverage - I cannot test that check but I feel uncomfortable with just removing it so I've ignored it for coverage.

What do you mean?

[jodator]

What do you mean?

@Reinmar: I've added this ignore

ckeditor5-utils/src/emittermixin.js

Lines 508 to 513 in f1a92c1

	function getCallbacksListsForNamespace( source, eventName ) {
	const eventNode = getEvents( source )[ eventName ];
	/* istanbul ignore if: this check make sense but in code it does not occur. */
	if ( !eventNode ) {
	return [];

Because it's private method that in code is used either with registering event namespace:

ckeditor5-utils/src/emittermixin.js

Lines 151 to 152 in f1a92c1

	createEventNamespace( emitter, event );
	const lists = getCallbacksListsForNamespace( emitter, event );

or is indirectly checked (as in removeCallback() ):

ckeditor5-utils/src/emittermixin.js

Lines 203 to 204 in f1a92c1

	if ( !emitters || ( emitter && !emitterInfo ) || ( event && !eventCallbacks ) ) {
	return;

In other words I'm not able to make a test for this one line.

[jodator]

That line could be removed though - It's not covered in test for above reason - it is superflous in current code. I've done some monkey clicking/typing on editor and it seems that that part of code is just dead...

[Reinmar]

An unnecessary code should be removed. There should be no zombie code. That the first thing.

The second thing is – how did you check that it's really unnecessary? There were very little tests for these scenarios. So, this should be validated by writing tests, not assuming (which even when correct, is short-termed).

I did both things in 4e3c1e9.

Then, I turned back to tests. As I wrote – a change in the code must be accompanied by tests (unless it's internal). This change is not internal. You're fixing a bug here. Changing the behaviour of this mixin. This must be tested. Not once, manually. But by automated tests.

I verify whether a change is tested like this:

A test should be failing when I checked out src/ to master.

[jodator]

@Reinmar I've added a bit more tests and it looks that mentioned code was indeed needed - My bad :(.

Right now there are two failing tests both might be considered bugs or features:

1. adding empty callback will break fire()

listner.listenTo( emitter, 'foo' );
emitter.fire( 'foo' );

2. once() handling general events:

emitter.once( 'foo', doSomething );

emitter.fire( 'foo:bar' );
// will call doSomething

emitter.fire( 'foo:bar' );
// will call doSomething

[jodator]

I've also included some more tests (mixed listeners) for dom/EmitterMixin but I can see that it had all cases covered already.

[Reinmar]

The build fails.

[jodator]

@Reinmar I've forgot to talk about it but to tests fails:

1. undefined callback in listenTo - either bug - either wrong use of API. I'd propose to add safety check to not blow up things if some plugin adds empty plugin.

2. The once behavior is either OK either not OK.

Should it be called only once on namespaced events or not?

emitter.once( 'foo', spy );

emitter.fire( 'foo:bar' );
emitter.fire( 'foo:bar' );

// it will call `spy` indefinitely.

[Reinmar]

If you consider these topics unrelated to the changes that we're doing in this PR, then sure – go ahead. They seem unrelated to me, but I haven't tried to analyse it too long.

[jodator]

I've created: https://github.com/ckeditor/ckeditor5-utils/issues/204 & https://github.com/ckeditor/ckeditor5-utils/issues/203 for those issues.