Fix event delegation (§4)

[raphinesse]

From @raphinesse on July 5, 2018 8:40

Current situation

Right at the start, cordovaCreate calls the following function with the
extEvents argument passed to it.

/**
 * Sets up to forward events to another instance, or log console.
 * This will make the create internal events visible outside
 * @param  {EventEmitter} externalEventEmitter An EventEmitter instance that will be used for
 *   logging purposes. If no EventEmitter provided, all events will be logged to console
 * @return {EventEmitter}
 */
function setupEvents (externalEventEmitter) {
    if (externalEventEmitter) {
        // This will make the platform internal events visible outside
        events.forwardEventsTo(externalEventEmitter);
    // There is no logger if external emitter is not present,
    // so attach a console logger
    } else {
        CordovaLogger.subscribe(events);
    }
    return events;
}

Pain Points

• Every call to cordovaCreate w/out extEvents subscribes CordovaLogger to
  events again w/ no possibility to unsubscribe it.
• The forwarding we setup isn't scoped to cordovaCreate either.
• cordovaCreate is tightly coupled to the Cordova event bus singleton.
• We cannot even rely on the Cordova event bus to be a singleton. Modules can get different versions of cordova-common (e.g. during development with linked dependencies). Then we have multiple event buses with no sane way of choosing which one we want emit events on.

Proposal

The only sane way I see is to accept an EventEmitter as an option (just as we do now), and only emit events there:

const emit = extEvents
    ? (...args) => extEvents.emit(...args)
    : () => {};

This would solve all problems described above.

We should also consider applying this pattern to other Cordova components.

---

Evolved from apache/cordova-discuss#89

Copied from original issue: apache/cordova-discuss#101

[raphinesse]

From @brodybits on July 5, 2018 14:50

Before evaluating I would have to do some research to see how the other modules, other functions do similar functionality such as:

• show progress and warnings to the user if called by Cordova CLI
• send events to a custom EventEmitter
• use of CordovaLogger

I gotta say that this is starting to uncover some other lurking smells that should be noted and hopefully addressed at some point. For example:

• Issue with Cordova singletons due to how NPM dependency management works
• I do not find the layers of abstractions between packages such as cordova-common, cordova-create/fetch/...,, cordova-serve, cordova-lib, cordova-cli to be so intuitive. Maybe we can solve this by more clear documentation of the Cordova tooling architecture.

I would be happy to deal with some of these items in separate issues.

[raphinesse]

Before evaluating I would have to do some research to see how the other modules, other functions do similar functionality

I've dug into most of tooling by now and I didn't recognize any clear recurring pattern.

I gotta say that this is starting to uncover some other lurking smells that should be noted and hopefully addressed at some point. [...] I would be happy to deal with some of these items in separate issues.

I've encountered so many of those in the last weeks, it's a wonder I still have a sense of smell 😅
I just try to tackle these issues one at a time. I don't know if making a list of all of them is going to help, but it definitely won't hurt. I'm just not too eager to make one myself.

Issue with Cordova singletons due to how NPM dependency management works

It seems to work reasonably well most of the time, but it's definitely not perfect. Full Dependency Injection would help to solve that, but it's not a common pattern in the JavaScript eco-system AFAICT. Thus my proposal to accept an EventEmitter and use it when available. Simple and efficient.

I do not find the layers of abstractions between packages such as cordova-common, cordova-create/fetch/...,, cordova-serve, cordova-lib, cordova-cli to be so intuitive. Maybe we can solve this by more clear documentation of the Cordova tooling architecture.

I agree that it's not obvious. Most of it isn't perfect either. It just is what it is. It would definitely be nice to pull more things out of lib. But you just have to take this task one piece at a time. E.g. I would like to pull out the very few things that cordova-lib still does during cordova create into cordova-create and use the latter directly from cordova-cli.

I've made this when I started working on Cordova tooling:

I was meaning to add that to the apache/cordova repo, but I did not get to it yet.

[raphinesse]

From @brodybits on July 6, 2018 23:22

@raphinesse thanks for the response with the nice picture. I read through your response pretty quickly, hope to process it in more depth next week.