[loopingz]

Proposed Changes

• Update Emitter from deprecated to singleton paradigm
• Allow implementing EventEmitter
• It makes it cleaner as from any point in the source code you can just use .emit() of a CloudEvent to send it, you do not care about how it is sent to the right target

We might want to consider removing the EventEmitter and adding the listeners' management to be able to ensure async methods so we can emit and waiting for the event to be completely emitted before continue processing. (my favorite options will discuss it tomorrow during our call)

I was not able to avoid the warning for the <any> in the unit test, usage of unknown in the sdk is not helping there

[lholmquist]

I think one of the main reasons that we removed the Emitter, was to remove any dependency on external protocol libraries, in this case axios. This also becomes important when adding in new protocols, like MQTT, etc.. We only wanted this module to output the correct formatting and let the user decide which library to use to actually emit it.

I'm not sure how this accomplishes that task

[loopingz]

@lholmquist this is not at all pushing to any technologies, it just streamlines the way to send event inside an application, a programmer should not care or know about what its subscribers are to emit an event. Without this kind of logic, it is hard to implement a subscription API, as the subscription is dynamic so is the endpoints to send the event too.

You can see the example on https://gateway.cloudevents.loopingz.com

[loopingz]

As @grant comment, I will move from EventEmitter to static async functions

[loopingz]

So I added the on() as static to have a shortcut on the Emitter class
I kept the singleton but renamed it to instance, as a developer we do not need to get access to the singleton.

I added an option ensureDelivery to wait for all listener to process the event if set to true

[lance]

Looks good! I really like this idea and think it will work well in certain scenarios - especially the browser. 💯 I have a few minor requested changes, and one question about Promise handling in the emitter.

[lance]

LGTM but can you help me understand why this test is failing on GH Actions?

[grant]

Not a fan of ensureDelivery. I would expect no parameter. The Promise should fail if there is no delivery and the user should retry the delivery or handle it.

[loopingz]

The event system of node is not done this way, so if we follow normal events it will never fail, I think adding the option to ensure delivery is useful for some cases

[grant]

Sorry, I don't believe this change is beneficial. There are numerous issues I've pointed out in comments in a review, including changes not directly related to emitter functionality.

Main issue is that we removed a static emitter (mentioned here #366 (comment)) but we have a emitterFor function for this use-case.

[grant]

Modifying ESlint rules is great, but really out of scope for a PR about emitters.

I would imagine 2 PRs that are separate:

• One PR the rules for quotes/any.
• One PR that changes the emitter.

[lance]

I would imagine 2 PRs that are separate:

I think the point of this change is that the existing linter rules was causing this PR to generate warnings during the build. See #366 (comment). I'm not sure of the benefit returned by extracting this small change into a separate PR, landing that first, then landing this. In fact, I recall recently that you were not happy about the number of commits in this repo - this seems counter to that complaint! 😉

[grant]

Hey, I'd quickly approve a change/fix to the eslint rules, but I don't want to approve a change that includes while (msg === undefined) {. The eslint change is a different decision than emitter functionality and would like to be merged even if any emitter PRs were not merge. Does that make sense?

[lance]

The eslint change is a different decision than emitter functionality and would like to be merged even if any emitter PRs were not merge. Does that make sense?

Sure. It just seems pedantic to demand this.

[grant]

Lance, I'm not demanding. This is just a review suggestion.

[loopingz]

The goal was to merge the PR in a timely manner so at that time we thought it would make sense to have only one PR

[grant]

Having a PR reviewed in a timely manner is fair. In general, it would be easier to not go back and forth and review faster with a GitHub issue and small PR. There have been discussions about removing this before, and we just talked about splitting this repo into different modules, so it's difficult to approve this PR right away.

That said, this repo is likely going to change a bit with the split, and I don't want to block you if Lance approves.

[grant]

nit: Modifying the URLs and list markdown isn't really in scope for this Emitter PR

[lance]

I tend to agree with this, but don't find it objectionable.

[loopingz]

The markdown linter should be also set to avoid this in the future

[grant]

Sorry, this is not in scope with the PR.

[loopingz]

The export was added to be able to share some logics and avoid duplicated code

[lance]

The export was added to be able to share some logics and avoid duplicated code

Makes sense to me.

[grant]

Why are we creating an event listening system for an event emitter?

I do not expect an eventing system like this.

[loopingz]

This is a shortcut to avoid the getSingleton that you did not like, I started to reimplement the EventEmitter interface, but found it overkill, as there are several other methods that can be useful. So instead of reinventing the wheel, I just added a shortcut method to streamline the process

[lance]

In my opinion, the example added to the README is very illustrative. This PR makes it so that you can set up your emitter function using emitterFor(), then have that respond to all cloudevent events in the Node event system by actually sending the CloudEvent. To me that seems quite valuable. Imagine that you have a large system that is pushing out CloudEvents, perhaps even to multiple different endpoints. You could add multiple emitter functions at startup that each send to a different endpoint. Then by simply calling new CloudEvent({...}).emit(); the CloudEvents is sent - with that single call - to all endpoints. I like it a lot.

[loopingz]

Yes this was the exact purpose, thanks for summarizing it so well

[grant]

Yup, the README example is great.

const axios = require("axios").default;
const { emitterFor, Mode, CloudEvent, Emitter } = require("cloudevents");
function sendWithAxios(message) {
  // Do what you need with the message headers
  // and body in this function, then send the
  // event
  axios({
    method: "post",
    url: "...",
    data: message.body,
    headers: message.headers,
  });
}
const emit = emitterFor(sendWithAxios, { mode: Mode.BINARY });
// Set the emit
Emitter.on("cloudevent", emit);
...
// In any part of the code will send the event
new CloudEvent({ type, source, data }).emit();

I think the fundamental question is why are we adding back the emit function? And adding a ensureDelivery param?

A CloudEvent object and a CloudEvent emitter/receiver seem like separate concepts. So I wouldn't imagine a CloudEvent has a send/receive function.

const axios = require("axios").default;
const { emitterFor, Mode, CloudEvent, Emitter } = require("cloudevents");
function sendWithAxios(message) {
  // Do what you need with the message headers
  // and body in this function, then send the
  // event
  axios({
    method: "post",
    url: "...",
    data: message.body,
    headers: message.headers,
  });
}
const emit = emitterFor(sendWithAxios, { mode: Mode.BINARY });

// In any part of the code will send the event
const ce = new CloudEvent({ type, source, data });
emit(ce);

Folks can add an eventing layer on-top of the primitives here. Maybe point out an error in the above alternative code?

[loopingz]

The emitEvent function was adding because EventEmitter is not able to "wait" for all listeners to finish async jobs.
You might by design consider that you need to fail the whole process if you were not able to send the event, or you can decide to let it go depending on how critical to your business the CloudEvent is.
This is the SDK so I think it should be able to answer both needs instead of limiting to one interpretation.

[lance]

So I wouldn't imagine a CloudEvent has a send/receive function.

Just to be clear, we are talking about two separate concepts. It's easy to get things mixed up when all of the terms are so similar. The two separate things as I see it are:

1. Sending a CloudEvent over some network transport using the emitterFor() factory to create a user defined function that is capable of converting a CloudEvent into a Message and sending that over the wire with a network transport (typically HTTP).
2. Using the Node.js built in EventEmitter capabilities to notify a listener that a CloudEvent should be sent using the method described in 1) point above. The listener in this case is the transport function created with emitterFor(). The new CloudEvent#emit() function just facilitates sending these using the Emitter.emitEvent() function.

This pull request is about using what is described in 2) to facilitate and orchestrate 1) in an efficient way across a large system that may need to send CloudEvents over network transport to multiple endpoints and do so efficiently by making use of built in Node.js EventEmitter capabilities.

Maybe point out an error in the above alternative code?

This pull request is not an alternative to that code. It builds upon that code to add a very useful feature, in my opinion, with very low overhead using Node.js built in, idiomatic capabilities.

[grant]

The casts used in this test seem very fishy (casting to unknown, then an interface).

Additionally, there are no comments, making the tests hard to understand.

[loopingz]

That is why we removed the eslint rule according with @lance

[grant]

A while loop like this, waiting for a response, is really bad practice. I'd imagine an await pattern after emitting.

[loopingz]

I agree overall but...
The NodeJS eventing system is not designed with Promise https://nodejs.org/api/events.html

[grant]

I'm sorry, but there are better methods to test an asynchronous pattern than a while undefined loop. A while loop like this is blocking I/O.

If we did want to go this route, in Node, it's common to not write === false or === undefined, rather use !. If we can, I'd await the event.

[loopingz]

You mean this one: https://nodejs.org/api/events.html#events_events_on_emitter_eventname_options because it is not available on Node 10 which is one of the target

I do not know other way to get a promise on an event, how would you do it?

[loopingz]

Also, can you iterate on the blocking I/O as this loop include the await of timeout so I wonder why/how it is blocking the I/O.
Thanks for clarifying so I can get a better understanding of NodeJS

[grant]

Right now the on function implemented does not return a Promise. Promises are a good way to have async processing in general.

You could use the Events emitter.on pattern too. Whatever the choice, I would not have a while loop like this, or would imagine exponential backoff if required.

[loopingz]

The NodeJS pattern is available only on Node v12.16.0 so cannot be used.
I just updated the PR with a custom Promise that will resolve by the event itself mimicking a bit the pattern

[lance]

I wonder if this shouldn't default to true. Doesn't it seem that it would typically be bad practice to "fire and forget"?

Suggested change

	public async emit(ensureDelivery = false): Promise<this> {
	public async emit(ensureDelivery = true): Promise<this> {

[loopingz]

I put false by default as this is the behavior of EventEmitter, I suppose both are defendable, if you have several subscribers maybe only is critical.
I suppose in my future implementation I would go for true with the main subscriber and optional one

[lance]

Maybe some more descriptive text for the test case would be good.

Suggested change

	it("emit", async () => {
	it("emits a Node.js 'cloudevent' event as an EventEmitter", async () => {

[loopingz]

It is part of the wider group EventEmitter

[grant]

This repo is likely going to change a bit with the split we talked about, and I don't want to block emitting efforts. Will dismiss my review.

That said, I think more discussion about event listeners / lifecycle methods in general in the CE SDKs would probably be worthwhile next call. I hope the points I made are clear.

static on(event: "cloudevent" | "newListener" | "removeListener", listener: (...args: any[])

Not going to block change. Deferring to Lance.

[loopingz]

I think we are all set, thanks gentlemen

@grant, sure let's discuss this longer in our call, we can still fix or improve things afterward

I, on purpose, split into two commits to avoid having the linter modification blended in the feature, so please do a not splash commit on merge