Emitter #366

loopingz · 2020-11-12T00:30:06Z

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 · 2020-11-12T16:17:54Z

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 · 2020-11-12T16:33:15Z

@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 · 2020-11-12T18:50:16Z

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

loopingz · 2020-11-12T20:40:47Z

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.

src/event/cloudevent.ts

src/transport/emitter.ts

lance

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

.eslintrc

test/integration/emitter_singleton_test.ts

Signed-off-by: Remi Cattiau <[email protected]>

grant · 2020-11-13T20:02:10Z

src/event/cloudevent.ts

+   * @param {boolean} ensureDelivery fail the promise if one listener fail
+   * @return {Promise<CloudEvent>} this
+   */
+  public async emit(ensureDelivery = false): Promise<this> {


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.

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 · 2020-11-13T20:30:34Z

.eslintrc

@@ -30,6 +30,7 @@
    }],
    "valid-jsdoc": "warn",
    "semi": ["error", "always"],
-    "quotes": ["error", "double", { "allowTemplateLiterals": true }]
+    "quotes": ["error", "double", { "allowTemplateLiterals": true }],


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.

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! 😉

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?

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.

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

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

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 · 2020-11-13T20:31:17Z

README.md

@@ -1,7 +1,7 @@
 # JavaScript SDK for CloudEvents

-[![Codacy Badge](https://api.codacy.com/project/badge/Grade/bd66e7c52002481993cd6d610534b0f7)](https://www.codacy.com/app/fabiojose/sdk-javascript?utm_source=github.com&amp;utm_medium=referral&amp;utm_content=cloudevents/sdk-javascript&amp;utm_campaign=Badge_Grade)
-[![Codacy Badge](https://api.codacy.com/project/badge/Coverage/bd66e7c52002481993cd6d610534b0f7)](https://www.codacy.com/app/fabiojose/sdk-javascript?utm_source=github.com&amp;utm_medium=referral&amp;utm_content=cloudevents/sdk-javascript&amp;utm_campaign=Badge_Coverage)
+[![Codacy Badge](https://api.codacy.com/project/badge/Grade/bd66e7c52002481993cd6d610534b0f7)](https://www.codacy.com/app/fabiojose/sdk-javascript?utm_source=github.com&utm_medium=referral&utm_content=cloudevents/sdk-javascript&utm_campaign=Badge_Grade)


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

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

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

grant · 2020-11-13T20:33:31Z

test/integration/emitter_factory_test.ts

+ * @param {Record<string, Record<string, string>>} response received to compare to fixture
+ * @return {void} void
+ */
+export function assertBinary(response: Record<string, string>): void {


Sorry, this is not in scope with the PR.

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

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

Makes sense to me.

grant · 2020-11-13T20:35:27Z

src/transport/emitter.ts

+   * @param {Function} listener to call on event
+   * @return {void}
+   */
+  static on(event: "cloudevent" | "newListener" | "removeListener", listener: (...args: any[]) => void): void {


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

I do not expect an eventing system like this.

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

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.

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

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?

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.

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:

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).

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 · 2020-11-13T20:37:57Z

test/integration/emitter_singleton_test.ts

+
+import { rejects, doesNotReject } from "assert";
+
+describe("Emitter Singleton", () => {


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.

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

grant · 2020-11-13T20:38:52Z

test/integration/emitter_singleton_test.ts

+    const emitter = emitterFor(fn, { binding: HTTP, mode: Mode.STRUCTURED });
+    Emitter.on("cloudevent", emitter);
+    fixture.emit();
+    while (msg === undefined) {


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

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

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.

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?

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

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.

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 · 2020-11-13T21:56:33Z

src/event/cloudevent.ts

+   * @param {boolean} ensureDelivery fail the promise if one listener fail
+   * @return {Promise<CloudEvent>} this
+   */
+  public async emit(ensureDelivery = false): Promise<this> {


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> {

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 · 2020-11-13T21:59:46Z

test/integration/emitter_singleton_test.ts

+import { rejects, doesNotReject } from "assert";
+
+describe("Emitter Singleton", () => {
+  it("emit", async () => {


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 () => {

It is part of the wider group EventEmitter

grant · 2020-11-13T22:27:28Z

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.

Signed-off-by: Remi Cattiau <[email protected]>

loopingz · 2020-11-13T22:40:25Z

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

loopingz force-pushed the emitter branch from 3876add to e9fc7be Compare November 12, 2020 00:40

lholmquist requested review from lance, grant and lholmquist November 12, 2020 16:13

loopingz force-pushed the emitter branch from 8b0e778 to 1671f66 Compare November 12, 2020 16:39

loopingz marked this pull request as draft November 12, 2020 18:53

loopingz force-pushed the emitter branch from bb67651 to 6a9e4ed Compare November 12, 2020 20:26

loopingz marked this pull request as ready for review November 12, 2020 20:40

lance requested changes Nov 12, 2020

View reviewed changes

loopingz force-pushed the emitter branch 6 times, most recently from 888b328 to 4e2d2b5 Compare November 12, 2020 23:46

loopingz requested a review from lance November 12, 2020 23:49

lance reviewed Nov 13, 2020

View reviewed changes

.eslintrc Show resolved Hide resolved

test/integration/emitter_singleton_test.ts Show resolved Hide resolved

test/integration/emitter_singleton_test.ts Outdated Show resolved Hide resolved

lance mentioned this pull request Nov 13, 2020

Deprecated API transition guide. #363

Merged

loopingz added 3 commits November 13, 2020 11:34

feat: add EventEmitter to Emitter and singleton paradigm

309ef4b

Signed-off-by: Remi Cattiau <[email protected]>

docs: add Emitter logic example

4697768

Signed-off-by: Remi Cattiau <[email protected]>

ci: add unit test for emitter

c98313b

Signed-off-by: Remi Cattiau <[email protected]>

loopingz force-pushed the emitter branch 2 times, most recently from 8acecc9 to 3868898 Compare November 13, 2020 19:41

grant reviewed Nov 13, 2020

View reviewed changes

grant previously requested changes Nov 13, 2020

View reviewed changes

lance approved these changes Nov 13, 2020

View reviewed changes

lance reviewed Nov 13, 2020

View reviewed changes

loopingz force-pushed the emitter branch from 3868898 to 9229201 Compare November 13, 2020 22:04

loopingz added 2 commits November 13, 2020 14:37

feat: allow ensureDelivery to be able to ensure delivery on emit

e7a36d3

Signed-off-by: Remi Cattiau <[email protected]>

ci: disable @typescript-eslint/no-explicit-any linter rule

6267205

Signed-off-by: Remi Cattiau <[email protected]>

loopingz force-pushed the emitter branch from 9229201 to 6267205 Compare November 13, 2020 22:38

lance merged commit 6adb578 into cloudevents:main Nov 17, 2020


		import { rejects, doesNotReject } from "assert";

		describe("Emitter Singleton", () => {

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

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

Emitter #366

Emitter #366

Uh oh!

Conversation

loopingz commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

lholmquist commented Nov 12, 2020

Uh oh!

loopingz commented Nov 12, 2020

Uh oh!

loopingz commented Nov 12, 2020

Uh oh!

loopingz commented Nov 12, 2020

Uh oh!

lance left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lance left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lance Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grant Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

loopingz commented Nov 12, 2020 •

edited

Loading

lance Nov 13, 2020 •

edited

Loading

grant Nov 13, 2020 •

edited

Loading