Skip to content

Emitter Should Optionally Be Static #314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
grant opened this issue Aug 11, 2020 · 30 comments
Closed

Emitter Should Optionally Be Static #314

grant opened this issue Aug 11, 2020 · 30 comments

Comments

@grant
Copy link
Member

grant commented Aug 11, 2020

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) => {}))
@lance
Copy link
Member

lance commented Aug 11, 2020

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.

@matejvasek
Copy link
Contributor

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.

@grant
Copy link
Member Author

grant commented Aug 11, 2020

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.

@lholmquist
Copy link
Contributor

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

@grant
Copy link
Member Author

grant commented Aug 11, 2020

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.

@matejvasek
Copy link
Contributor

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

@lance
Copy link
Member

lance commented Aug 11, 2020

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.

@grant
Copy link
Member Author

grant commented Aug 11, 2020

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.

@matejvasek
Copy link
Contributor

matejvasek commented Aug 11, 2020

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

@matejvasek
Copy link
Contributor

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

@grant
Copy link
Member Author

grant commented Aug 11, 2020

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.

@lance
Copy link
Member

lance commented Aug 12, 2020

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.

@matejvasek
Copy link
Contributor

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" })

@lholmquist
Copy link
Contributor

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

@grant
Copy link
Member Author

grant commented Aug 12, 2020

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

@grant grant changed the title Emitter Should Be Static Emitter Should Optionally Be Static Aug 21, 2020
@github-actions
Copy link
Contributor

This issue is stale because it has been open 30 days with no activity.

@lholmquist
Copy link
Contributor

I think we can close this since the re-write that will be released as 4.x should address this.

@grant @lance does that sound correct?

@grant
Copy link
Member Author

grant commented Sep 21, 2020

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

@lance
Copy link
Member

lance commented Sep 22, 2020

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

@lholmquist
Copy link
Contributor

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

@lholmquist
Copy link
Contributor

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

@lance
Copy link
Member

lance commented Sep 28, 2020

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.

@grant
Copy link
Member Author

grant commented Sep 28, 2020

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.

export class Emitter {

@lance
Copy link
Member

lance commented Sep 28, 2020

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

@grant
Copy link
Member Author

grant commented Sep 28, 2020

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.

@grant
Copy link
Member Author

grant commented Sep 28, 2020

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

@lance
Copy link
Member

lance commented Sep 28, 2020

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?

@grant
Copy link
Member Author

grant commented Sep 28, 2020

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

@lance
Copy link
Member

lance commented Sep 29, 2020

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

@grant
Copy link
Member Author

grant commented Sep 29, 2020

Looks great, will close this issue

@grant grant closed this as completed Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants