Simplify Binary/Structured Receiver

[grant]

Describe the Bug

I'd expect the Binary/Structured Receiver to be really easy to setup. A developer shouldn't need to know about what mode the CloudEvent is in when receiving.

These methods should be static too.

Right now there's a lot of complicated logic for the binary/structured HTTP receiver. They should probably be 1 file each.

[lance]

This is why we export Receiver and Emitter now. Developers are not required to use BinaryReceiver or StructuredReceiver.

https://cloudevents.github.io/sdk-javascript/classes/emitter.html
https://cloudevents.github.io/sdk-javascript/classes/receiver.html

[lance]

Right now there's a lot of complicated logic for the binary/structured HTTP receiver. They should probably be 1 file each.

What specifically do you have a problem with?

[grant]

What specifically do you have a problem with?

• Receiver is an object, when it should be static.
• There is no generic HTTP receiver. Binary and Structured receivers should really just be combined. This is how some other SDKs work. A consumer doesn't know if a CE is binary/structured before it is sent to the server.
  • https://github.com/cloudevents/sdk-javascript/blob/master/src/transport/http/binary_receiver.ts
  • https://github.com/cloudevents/sdk-javascript/blob/master/src/transport/http/structured_receiver.ts

Really, we should make receiving a CloudEvent easier like is done in python with a simple function:
https://github.com/cloudevents/sdk-python/tree/v1.0.0-dev#request-to-cloudevent

[grant]

If we export the BinaryHTTPReceiver in addition to Receiver, we can't stop folks from using those.

We should probably get rid of them in favor of just Receiver.

[lance]

These classes are essentially the implementation for Receiver. I don't want to get rid of them. And I think exposing them is not inherently wrong.

[grant]

Why do we want both BinaryHTTPReceiver and StructuredHTTPReceiver?
We can just have 1 receiver that accepts both binary and structured CloudEvents. What do you think of that?

[lance]

We can just have 1 receiver that accepts both binary and structured CloudEvents. What do you think of that?

That is Receiver. It exists. We don't need to make it again.

[grant]

It's frustrating talking about this. There shouldn't be multiple ways of doing things.

[n3wscott]

It's frustrating talking about this. There shouldn't be multiple ways of doing things.

I do not feel this is a productive comment.

The SDK should take no opinion on optimization paths. Receiver is the general usecase but for someone who wants to write a service that optimizes for accepting only binary, or only structured events and wants to short-circuit the parsing path, it seems reasonable to expose this underlying detail.

The SDK should leave as many hooks and opening for interesting usecases, which can be more flexible than a function authoring library would like to expose.

Frustration based on arbitrary personal opinions should be left at the door, we are all trying to build some helpful generic software to aid in the adoption and usage of CloudEvents. Remember there are more usecases than just yours. It is a tricky balance in this work.

[grant]

The SDK should take no opinion on optimization paths. Receiver is the general usecase but for someone who wants to write a service that optimizes for accepting only binary, or only structured events and wants to short-circuit the parsing path, it seems reasonable to expose this underlying detail.

I'm not trying to optimize, I'm trying to be DRY/KISS as we have duplicative logic in these files:

• https://github.com/cloudevents/sdk-javascript/blob/master/src/transport/http/binary_receiver.ts
• https://github.com/cloudevents/sdk-javascript/blob/master/src/transport/http/structured_receiver.ts

Where do we say we need to reject encodings in the CESDK requirements? This is not implemented in other SDKs afaik.

Each SDK MUST support both HTTP transport renderings in both structured and binary encodings.

• https://github.com/cloudevents/spec/blob/master/SDK.md

I'm happy to adopt to lots of use-cases, really. LMK how this proposal doesn't accommodate.

[lance]

I'm not trying to optimize, I'm trying to be DRY/KISS as we have duplicative logic in these files:

Please feel free to contribute code to this project for community review in order to satisfy your demands.

Where do we say we need to reject encodings in the CESDK requirements? This is not implemented in other SDKs afaik.

Help me understand where you think the module is “reject[ing] encodings”. This code that you reference does not do such a thing.

[lance]

I'm going to go ahead and close this issue, as the one agreed upon step - making Receiver.accept() static - has landed. It's not clear to me what, if anything, else should be done. @grant if you have specific/tangible next steps, feel free to reopen and describe these, or create a new issue.

[grant]

That's fine. A lot of improvements have been made.

There's some internal refactoring for making static receivers (every require('cloudevents') per module shouldn't create 4 objects), but that's not super important right now.
https://github.com/cloudevents/sdk-javascript/blob/main/src/transport/receiver.ts#L16-L25

[lance]

There's some internal refactoring for making static receivers (every require('cloudevents') shouldn't create 4 objects, but that's not super important right now.

That's not how it works with require(). Those objects are created only once. Here is a simple example to illustrate.

[grant]

Updated the comment to "per module".

The above sample is requiring in the same npm module. Requiring cloudevents in separate modules creates separate objects/namespaces.

[lance]

The above sample is requiring in the same npm module. Requiring cloudevents in separate modules creates separate objects/namespaces.

After this, I'm going to drop it... but... you are mistaken. Please see the Node.js module documentation.

Modules are cached after the first time they are loaded. This means (among other things) that every call to require('foo') will get exactly the same object returned, if it would resolve to the same file.
Provided require.cache is not modified, multiple calls to require('foo') will not cause the module code to be executed multiple times.

There are some caveats to this. For example, if my project has multiple dependencies which each depend on a different version of cloudevents, those dependencies would resolve to different files and the file will be loaded and compiled multiple times. In most cases, however, this does not happen. Do an npm install on this repo, for example. All of the direct dependencies - as well as all of the transitive dependencies are at the top level of the node_modules directory. So every time, require('is-utf8') (for example) is called by any of the project's dependencies, it will resolve to this top level module directory. FWIW, I have actually implemented a Node.js compatible require() in a JavaScript runtime, so I'm not just guessing here.