-
Notifications
You must be signed in to change notification settings - Fork 69
feat: add emitterFactory and friends #342
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
Conversation
This commit adds an emitterFactory function that returns an EmitterFunction object. The EmitterFunction may be used to emit events over a supported network transport layer. Currently, only HTTP is supported. Parameters provided to the emitterFactory are the transport Binding (only HTTP supported), the encoding mode (Mode.BINARY or Mode.STRUCTURED), and a TransportFunction. The implementation for emitBinary and emitStructured has been replaced with this simple pattern and those two functions have been removed. Example: ```js // The endpoint URL that will receive the event const sink = 'https://my-event-sink'; // A function that uses Axios to send a message over HTTP function axiosEmitter(message: Message, options?: Options): Promise<unknown> { return axios.post(sink, message.body, { headers: message.headers, ...options }); } // Create an event emitter const emit = emitterFactory(HTTP, Mode.BINARY, axiosEmitter); // Emit an event, sending it to the endpoint URL emit(new CloudEvent{ source: '/example', type: 'example' }); ``` Signed-off-by: Lance Ball <[email protected]>
Note: added tests include testing for Axios, SuperAgent and Got as HTTP transport libraries. |
@lance Does it make sense to only have 1 parameter(an option object) passed to the emitterFactory? Then we could easily default values. In node core, we've done that with some functions. |
@lholmquist const emit = emitterFactory({ binding: HTTP, mode: Mode.BINARY, transport: axiosEmitter }); With const emit = emitterFactory( { transport: axiosEmitter } ); Personally, I think I'd prefer two parameters: the function and an options object. E.g. const emit = emitterFactory( axiosEmitter, { binding: HTTP, mode: Mode.BINARY }); Which makes the default call a little cleaner. const emit = emitterFactory( axiosEmitter ); |
I can appreciate some of the usefulness for this utility, but I don't think a stateful emitter factory should be part of the SDK. It's really hard to discuss design decisions in the abstract, so in the PR description, can you write up the before and after with these changes? I think I can suggest a different way to achieve the same result without a factory. I can imagine a developer writing their own factory pattern that is even more flexible than the interface here. For example, a user could just write a wrapper for the emitter function and add extra params. In terms of interface design, Factories are more common in Java and less so in Node. Related: #314 – I really think we can allow the factory pattern wrapping an emitter. |
@grant the purpose of this PR is to provide a mechanism in 4.x for users to send events across the wire without this module having an explicit dependency on axios (or any other transport module). The
The factory is not stateful. But maybe you mean the emitter itself. As I see it, if we want the user to not have to provide the transport binding and serialization mode with every call there must be some state. Otherwise, with each function call that sends an event, the user would be required pass
Done.
OK - let's hear it.
The whole point is that the
Ahh but using closures to maintain state is quite a popular JavaScript style. I thought you'd like it. How about I rename it to |
I'd rather expose just 1 interface for users to emit CloudEvents. An emitter factory can be a fairly simple sample rather than in the SDK. It's a lot more customizable that way. User-defined code. Example: // User-defined emitter class. Not in the SDK
export emitterFactory(options) {
return {
send: (ce, customOptions) => {
const combinedOptions = {...options, ...customOptions};
Emitter.send(ce, combinedOptions);
}
};
} Ideally it's pretty simple to emit a CloudEvent. Rough interface (you can extend the transport, mode, etc.): // Static emitter
Emitter.send(ce, "https://cloudevents.io/example");
// Or another style (pseduocode)... ideally only one of these is implemented, this style is more functional. Above is more object oriented.
import {emit} from 'cloudevents/transports/http';
emit(ce, options); |
I'm not proposing more than 1 interface. As I have said multiple times,
This means that we would not be removing the dependency on axios. Is that what you are suggesting? I understood that removing this dependency is a goal for the v4.0.0 release. |
No. Removing that dep on a transport library is great. I wasn't complete with the samples. I guess why do we have a custom emitter at all? We should let users BYO / use your own libraries for emitting using raw primitives of HTTP (headers and body). Because we're removing the transport library, having a param inserting a transport library seems confusing. It seems like an extra interface that we don't really need. A user can just use transport library as they want. Example: const axios = require('axios').default;
const ce = { ... }
const [headers, body] = CloudEvents.asHttpRequest(ce);
axios({
method: 'post',
url: '...',
data: body,
headers,
}); Creating a wrapper emitter doesn't seem super useful, given there are dozens of transport libraries. Some transports don't have binary events for example, so I don't know why we are adding Hope you get my general perspective. |
I'm not sure what you mean by "custom emitter". This PR is about removing opinionated choices like the use of axios to send messages across the wire, and replacing it with something that is less "custom".
That is already possible. Perhaps you don't recall some of the conversations around this, or are not familiar with the codebase. It landed in this PR. Your hypothetical is available today, it just looks a little different. 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,
}); This PR is about providing users a way of encapsulating behavior. Imagine the user is not using axios, but instead their application has chosen superagent. Sending a import request from "superagent";
const ce = { ... }
const message = HTTP.binary(ce); // Or HTTP.structured(ce)
const post = request.post('...');
for (const key of Object.getOwnPropertyNames(message.headers)) {
post.set(key, message.headers[key]);
}
post.send(message.body); This PR would make that look a little different. // Same code as above but wrapped in a function
// User code - a TransportFunction
function sendViaSuperagent(message) {
const post = request.post('...');
for (const key of Object.getOwnPropertyNames(message.headers)) {
post.set(key, message.headers[key]);
}
post.send(message.body);
}
// The returned EmitterFunction handles converting from CloudEvent to Message and invokes 'sendViaSuperagent`
const emit = emitterFor(sendViaSuperagent);
const ce = { ... }
emit(ce); This encourages encapsulation. For example, Additionally, and probably more important, we have talked about adding additional transport modes support soon. This would mean that, for example, MQTT would look similar. // This is user code - a TransportFunction
function sendMQTT(message) {
// do whatever you need to do with message.body and message.headers to send it over MQTT
}
// Create an EmitterFunction
const emit = emitterFor(sendMQTT, { binding: MQTT });
const ce = { ... };
emit(ce); Note that this looks exactly the same as sending an event over HTTP. That's by design. In fact, I can use the Sure, it's a small benefit to the user to provide this. But I don't understand your hostility towards it, especially given that we can already do what you were asking for in the very trivial case of just sending an event with axios. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Lance, I hope you're not taking my comments as being hostile, it seems useful, I'm just trying to understand why the functionality has to live here (and should other languages have this feature).
Definitely not requesting changes. Thanks for the detailed explanations.
That works. |
No worries. I should have been more explicit in the PR description. |
Signed-off-by: Lance Ball <[email protected]>
I have incorporated feedback by renaming the function and changing up the parameters. function emitterFor(fn: TransportFunction, options = { binding: HTTP, mode: Mode.BINARY }): EmitterFunction PTAL |
This commit adds an
emitterFactory()
function that returns anEmitterFunction
. TheEmitterFunction
may be used to emit events over a supported network transport layer. Currently, only HTTP is supported.Parameters provided to the
emitterFactory()
function are the transportBinding
(only HTTP supported), the encoding mode (Mode.BINARY
orMode.STRUCTURED
), and aTransportFunction
. ATransportFunction
is a user supplied typed function that takes aMessage
andOptions
, and sends the event.The implementation for emitBinary and emitStructured has been replaced with this simple pattern and those two functions have been removed.
Example:
Per @grant
BEFORE
This is what we have today.
AFTER
The BEFORE functionality still exists until the release of 4.x at which time it will be removed. The proposed alternative below provides similar capability without an explicit dependency on a transport framework (e.g. axios).
Related: #314