Mirror events on an internal event handler

[rreusser]

Problem: Currently it is the responsibility of the item triggering an event to iterate over all controls and update them. This seems like a rather unmaintainable and tightly coupled pattern.

Solution: This PR creates a second event handler which receives a copy of all events emitted on the the regular event handler. It does this so that internal parts of plotly can respond to events without adding events onto the user-facing event handler—which would otherwise break plotly if the user ever executed plotObj.removeAllListeners().

Note that it mirrors the events so that it's not necessary to emit two copies of every event.

This could otherwise be achieved with a flag that differentiates internal vs external event handlers (so that removeAllListeners would not break things), but that would be significantly more invasive.

Possible caveat: Plotly is designed from the ground up to wipe things out and start over. This PR should be usable when refactoring controls, but we may need to add an extra cleanup phase that detaches events.

[etpinard]

Will we ever need to call Events.triggerHandler on internal events?

[rreusser]

Perhaps not. I didn't fully understand the use-case of this. It appeared that it was for backward compatibility with earlier jquery-based events, but yeah, perhaps not.

[etpinard]

Here's the only reason (that I can think of) why we still have that trigger event block: https://plot.ly/javascript/hover-events/#triggering-hover-events

I'd vote for dropping it in v2.0.0 (cc @alexcjohnson)

So, I'd vote for not adding support for triggerHandlers for internal events?

[rreusser]

More specifically, It appear to me that triggerHandler was just a more jquery-like version of emit, but I could be wrong. If there's no use-case for it, I'm glad to remove. I have no preference of particular knowledge here, so I'll defer to you on this one.

[etpinard]

TODO

• Let's remove this block
• add comment on non-internal triggerHandler block mentioning that internal events don't support triggerHandler.

[alexcjohnson]

I'd vote for dropping it in v2.0.0

👍
In case there are people using it in the wild, it would be great if our v2 release notes could describe (or link to a description of) how to convert to the new form.

[etpinard]

I really like this idea. @rreusser 👍

[rreusser]

@etpinard I should add that I deliberately omitted a convenience function to trigger events on the internal handler (though you can still do that pretty trivially with gd._internalEv.emit). The idea is that the internal and external event handlers share the same events and really just exist to segregate the handlers.

[etpinard]

I should add that I deliberate omitted a convenience function to trigger events on the internal handler

Yeah, that would be great.

[rreusser]

No rush @etpinard but can you clarify/confirm whether changes are desired?

[rreusser]

Ohh, sorry, I get it. Dropping triggerHandlers so don't bother adding more features that reinforce its usage. Okay.

[etpinard]

Dropping triggerHandlers so don't bother adding more features that reinforce its usage

exactly 👍

[etpinard]

Looks good 💃