Emitter Should Optionally Be Static

Describe the Bug

The HTTP Emitter should have static methods such that we don't need to construct multiple Emitters before sending our CE.

Currently we must instantiate the Emitter with a URL and then emit. See README.

const { CloudEvent, Emitter } = require("cloudevents");

const ce = new CloudEvent({ type, source, data });
async function test() {
  const emitter = new Emitter({
    url: "https://cloudevents.io/example"
  });
  await emitter.send(ce);
}

Expected Behavior

Like the static Receiver.accept(...), I'd imagine an Emitter.send(...).

const { CloudEvent, Emitter } = require("cloudevents");

const ce = new CloudEvent({ type, source, data });
async function test() {
  await Emitter.send(ce, "https://cloudevents.io/example");
}

This format is more idiomatic and is similar to the format for what other npm modules do:

http Standard library – (http.request(url, options))
axios - (axios.post(url, options))
request - (request.post({ url, ...options }))
superagent - (superagent.post(url).send(body).end((err, res) => {}))

I see where you are coming from with the desire for a static function. However, it's not the case that you are required to create a new Emitter for every URL. You can in fact, include a URL in the send call. For example.

const emitter = new Emitter(( { url: 'https://primary.system.com' });
emitter.send(new CloudEvent({ source: '/event-generator', type: 'system:info' });

// then later, maybe we send only error events to another URL
emitter.send(new CloudEvent({ source: '/event-generator', type: 'system:error'}, { url: 'https://error.system.com' });

This was designed intentionally so that the user isn't required to provide a URL for each send. In many cases, a system will have a single endpoint, and having the URL in the constructor enables this. I would argue that this is preferable to a static method if the static method always requires the same URL.

I just used javascript-sdk marginally, so correct me if I am wrong. I would expect emitter to contain some http-client object inside. Sometimes such an object my store some state, e.g. keep http-cookies. For that reason I would avoid having single static(global) emitter.

Creating a managed Emitter object is fine, but there are many use-cases where I don't want to manage state and figure out which file contains my emitter.

An alternative would be to have something like this:

const url = 'https://primary.system.com';
Emitter.send(new CloudEvent({ source: '/event-generator', type: 'system:info' }, { url }));

// then later, maybe we send only error events to another URL
Emitter.send(new CloudEvent({ source: '/event-generator', type: 'system:info' },  { url: 'https://error.system.com' })

Maybe we can we have both if we want to have a non-static emitter class? I see value in setting up configs for the emitter once.

trying to do both would be very confusing for the user.

OK. Can we just have 1 function to send a HTTP request? Like:

Emitter.send(url, options)

This format is more idiomatic and is similar to the format for what other npm modules do:

http Standard library – (http.request(url, options))
axios - (axios.post(url, options))
request - (request.post({ url, ...options }))
superagent - (superagent.post(url).send(body).end((err, res) => {}))

They don't require to create a class/object first.

I don't see exactly what useful state we're trying to preserve for the user in TransportOptions. I'm not sure how to keep http-cookies or other cases I might be missing mentioned above.

@grant

@grant Seems you are right that the JS/TS HTTP clients are mostly stateless (which is really good thing). I had different experience in different languages.

The current design is meant for simplicity once an Emitter has been created. TransportOptions holds a URL, the transport format (Binary or Structured) and the actual emitter implementation function (binaryEmitter or structuredEmitter). The idea is that, in most cases, you're going to be consistently sending an event using a single transport protocol format to a single URL. Instead of having to supply the transport format and URL with every call to emit(), this state is held for you in the class.

It's easy enough to default to one format or the other, but let's say the default is Protocol.HTTPStructured. If I have an event receiver that I want to be sending binary events to, that means I'd need to include this information in every call to emit().

// For example, if the default is structured, every call to a single endpoint would look like this
Emitter.emit( event, { protocol: Protocol.HTTPBinary, url: 'https://my-static.receiver.com' });

For me, it makes the most sense to have a class which contains the state of the endpoint (url, transport protocol, and an actual emitter implementation function), rather than having to specify this in the call to emit() every time. In most cases, an event emitter in a running processes will likely be always sending using the same transport protocol format and to the same URL.

Let's say I have some codebase that has several different branches where an event needs to be emitted to a broker that reads structured events. I'd say that doing this:

// specifiy these things once in the ctor
const emitter = new Emitter({ url: 'https://my.system.broker', protocol: Protocol.HTTPStructured });

// now everywhere in my code that this emitter is available just has to call
emitter.emit(event);

With your suggestion, this emitter.emit(event) call would need to look like this everywhere.

Emitter.emit( event, { url: 'https://my.system.broker', protocol: Protocol.HTTPStructured });

In my mind that's a lot more room for error, a lot more typing, and generally not as "nice" for a developer. I think that having an object that maintains this state, with the option to override it in subsequent calls to emit() is the more elegant and flexible solution.

I hear where you're coming from. The difference is a stateless functional vs stateful object-oriented design.

I'm suggesting to be more functional like Node:

Current

const emitter = new Emitter({ url: 'https://my.system.broker', protocol: Protocol.HTTPStructured });
emitter.emit(event);

Proposed

const options = { url: 'https://my.system.broker', protocol: Protocol.HTTPStructured };
Emitter.emit(event, options);

There are benefits for both styles of programming.

If you wanted to have a stateful endpoint, you can do this:

Current

const emitter = new Emitter({ url: 'https://my.system.broker', protocol: Protocol.HTTPStructured });
export emitter;

// ... other file
const emitter = require('./emitter');
emitter.emit(event);

vs

const emitterOptions = { url: 'https://my.system.broker', protocol: Protocol.HTTPStructured };
export emitterOptions;

// ... other file
const emitterOptions = require('./emitter');
Emitter.emit(event, emitterOptions);

If we need to change the protocol of one of these files, it's easy when there is a static emitter and harder with an Emitter object.

For example, let's say we want to now emit binary CEs. It's easy in the 2nd example:

Emitter.emit(event, {...emitterOptions, protocol: Protocol.HTTPBinary});

If we're really using emit everywhere, it's going to be a pain to manage an instance of an Emitter. We have to import it from one specific file vs being able to require('cloudevents') anywhere. Not managing state is a powerful part of the npm module ecosystem.

I think that instance of Emitter is supposed to be used only locally in single file (or even function) and not being exported.

I like stateless immutable things, but it would force user to repeat code like

const send = (e: CloudEvent, opts: TransportOptions = {}) => Emitter.send(e, { url: receiver, ...opts });

matejvasek@f86388a

or maybe it could be extracted:
matejvasek@dcdb24d#diff-c6d9cc63d2d9557f6593b77a2153144eR62

I like stateless immutable things, but it would force user to repeat code like
const send = (e: CloudEvent, opts: TransportOptions = {}) => Emitter.send(e, { url: receiver, ...opts });
matejvasek@f86388a

or maybe it could be extracted:
matejvasek@dcdb24d#diff-c6d9cc63d2d9557f6593b77a2153144eR62

You wouldn't need to repeat the code, you'd just need to export that stateful send function and use it within code if you really want to customize transport options.

I'm also not saying that they have to be mutually exclusive, if we want to keep state, we could have that too.

This is not a hill I feel like fighting on, so if you would like to propose an implementation in a PR that would be welcome. And we can discuss the merits of it there. I can see your point but do still have reasons to prefer the implementation as it is. Regarding idiomatic JS, there is a lot of room for interpretation around that and maintaining state is not inherently wrong in JS - note the recent addition of class to the language. JS is quite flexible and many developers use it in lots of different ways. This is one of the things that I love about the language... it’s flexible and expressive in lots of different ways.

What about adding some default immutable instance as static member:

class Emitter {
// ...
  static readonly default = Object.freeze(new Emitter({ url: "http://localhost:8080", protocol: Protocol.HTTPBinary }));
// ...
}

Then user could call:

Emitter.default.send(cloudEvent, { url: "example.com" })

@grant

@grant i would suggest to send a PR with the proposed changes. Maybe seeing the code in context might help

@matejvasek

Thanks for the concrete suggestion @matejvasek. I'll look at changing the PR to something like that.

Not sure of the status of this or what 4.x brings.

@grant

@grant the proposal was that Emitter is being deprecated because of the work that was done with the Message and Binding interfaces, etc. But, I've been thinking about this. I think before we drop 4.x, we should have a good, easy to use replacement, because it really does make life easy. Here is a prime example in the Knative docs (using the 2.x API).

@lholmquist are you good with holding off a bit on 4.x? I'm noodling something like an EmitterFactory that takes a Binding, SinkUrl, Mode and a function and does the right thing. I'm a little stalled on it at the moment. Will try to get a WIP PR submitted soonish.

@lance

@lance sounds good. i don't think we are in any rush for the 4.0 release

Now that #342 has been merged, should we close this?

Now that #342 has been merged, should we close this?

As you know, #342 did not create a static emitter. But I think the design direction we are taking with that PR makes this issue obsolete.

Now that #342 has been merged, should we close this?

Well, there's no way to send a CloudEvent without creating a factory or instance of an Emitter, so not really.

sdk-javascript/src/transport/emitter.ts

Line 97 in e334b6e

export class Emitter {

Well, there's no way to send a CloudEvent without creating a factory or instance of an Emitter, so not really.

Didn't we just go through all of this last week? There will be no Emitter.send(event) functionality in 4.x. Instead it will be const emit = emitterFor(myTransport);.

Hey, I was asked if we should close this issue. #342 creating a factory is useful but I thought it was a bit tangential. Really the goal was to not have to maintain an emitter across multiple files with a static option and just call emit anywhere (passing a transport).

The current interface is:

emitterFor(fn, transportOptions) // new
new Emitter(transportOptions) // standard

I guess I was expecting a static option like this:

Emitter.send(event, transportOptions) //static

so maybe a function like this here:

static send(event: CloudEvent, options: TransportOptions): Promise<unknown> {
  ...
}

with required options.

To be clear, I'm not expecting anybody to work on this. If we do have an emitter, a static option would be nice.

Or if we're removing the Emitter class, then just directly exporting the emit function statically would work.

Or if we're removing the Emitter class, then just directly exporting the emit function statically would work.

Yes, the plan is to remove Emitter.

so maybe a function like this here:
static send(event: CloudEvent, options: TransportOptions): Promise<unknown> {
  ...
}
with required options.

I don't understand how this would work, since we will be removing axios when we remove Emitter. Without axios, how would a static function like this actually send the event?

I guess I don't understand the best way to send an event currently.

The thinking I was going with was to pass a function (which can use axios or something else) to actually send the event, but that's a bit overkill.

Really, it would be nice to have a sample in the README like this:

const request = require('request');
import {toHTTPRequest} from 'cloudevents';

const ce = { id: 1234, .... };

const [headers, body] = toHTTPRequest(ce, { mode: Mode.BINARY }); // or Mode.STRUCTURED
request.post(url, headers, body);

This is similar to how python allows you to send requests: https://github.com/cloudevents/sdk-python#binary-http-cloudevent

I guess I don't understand the best way to send an event currently.

Outside of the Emitter class which is going away, there are two ways to send events. One is similar to the example you provide. E.g.

const axios = require('axios').default;

const ce = { ... }
const message = HTTP.binary(ce); // Or HTTP.structured(ce)

axios({
  method: 'post',
  url: '...',
  data: message.body,
  headers: message.headers,
});

And the other is what you suggested with passing a function. This just landed in #342.

The thinking I was going with was to pass a function (which can use axios or something else) to actually send the event

function sendViaSuperagent(message) {
  // send the message.headers and message.body across the wire 
}

const emit = emitterFor(sendViaSuperagent);

const ce = { ... }
emit(ce);

Looks great, will close this issue

Navigation Menu

Repository navigation

Emitter Should Optionally Be Static #314

Current

Proposed

Current

4 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Emitter Should Optionally Be Static #314

Description

Activity

lance commented on Aug 11, 2020

matejvasek commented on Aug 11, 2020

grant commented on Aug 11, 2020

lholmquist commented on Aug 11, 2020

grant commented on Aug 11, 2020

matejvasek commented on Aug 11, 2020

lance commented on Aug 11, 2020

grant commented on Aug 11, 2020

Current

Proposed

Current

matejvasek commented on Aug 11, 2020

matejvasek commented on Aug 11, 2020

grant commented on Aug 11, 2020

lance commented on Aug 12, 2020

matejvasek commented on Aug 12, 2020

lholmquist commented on Aug 12, 2020

grant commented on Aug 12, 2020

4 remaining items

grant commented on Sep 21, 2020

lance commented on Sep 22, 2020

lholmquist commented on Sep 23, 2020

lholmquist commented on Sep 28, 2020

lance commented on Sep 28, 2020

grant commented on Sep 28, 2020

lance commented on Sep 28, 2020

grant commented on Sep 28, 2020

grant commented on Sep 28, 2020

lance commented on Sep 28, 2020

grant commented on Sep 28, 2020

lance commented on Sep 29, 2020

grant commented on Sep 29, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions