Simplify Binary/Structured Receiver #273

Activity

1. lance commented on Jul 27, 2020

   

   lance

   on Jul 27, 2020

   Last edited by lance

   Member

   More actions

   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
2. lance commented on Jul 27, 2020

   

   lance

   on Jul 27, 2020

   Member

   More actions

   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?
3. grant commented on Jul 27, 2020

   

   grant

   on Jul 27, 2020

   Last edited by grant

   MemberAuthor

   More actions

   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
4. grant commented on Jul 27, 2020

   

   grant

   on Jul 27, 2020

   MemberAuthor

   More actions

   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.
5. lance commented on Jul 27, 2020

   

   lance

   on Jul 27, 2020

   Member

   More actions

   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.
6. grant commented on Jul 28, 2020

   

   grant

   on Jul 28, 2020

   MemberAuthor

   More actions

   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?
7. lance commented on Jul 28, 2020

   

   lance

   on Jul 28, 2020

   Member

   More actions

   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.
8. grant commented on Jul 28, 2020

   

   grant

   on Jul 28, 2020

   MemberAuthor

   More actions

   It's frustrating talking about this. There shouldn't be multiple ways of doing things.
9. n3wscott commented on Jul 29, 2020

   

   n3wscott

   on Jul 29, 2020

   Member

   More actions

   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.
10. grant commented on Jul 29, 2020

    

    grant

    on Jul 29, 2020

    MemberAuthor

    More actions

    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.
11. lance commented on Jul 30, 2020

    

    lance

    on Jul 30, 2020

    Member

    More actions

    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.
12. lance

    added

    status/stalledIssues that have not made progress recentlyIssues that have not made progress recently

    on Aug 17, 2020
13. lance commented on Aug 17, 2020

    

    lance

    on Aug 17, 2020

    Member

    More actions

    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.
14. lance

    closed this as completedon Aug 17, 2020
15. grant commented on Aug 17, 2020

    

    grant

    on Aug 17, 2020

    Last edited by grant

    MemberAuthor

    More actions

    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
16. lance commented on Aug 17, 2020

    

    lance

    on Aug 17, 2020

    Member

    More actions

    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.
17. grant commented on Aug 17, 2020

    

    grant

    on Aug 17, 2020

    MemberAuthor

    More actions

    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.
18. lance commented on Aug 17, 2020

    

    lance

    on Aug 17, 2020

    Member

    More actions

    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.