Skip to content
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

WebSockets protocol binding #697

Merged
merged 6 commits into from
Oct 8, 2020

Conversation

slinkydeveloper
Copy link
Member

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Fix #693

This PR adds a WebSockets protocol binding. The big 2 things this spec defines are:

  • Events are sent serialized in websocket messages, using an event format agreed during the handshake.
  • How to agree on the event format. tl;dr we just use subprotocols defining a mapping between websockets subprotocols and event formats

Sample implementation

I tested this protocol binding hacking the sdk-javascript sample here: https://github.com/cloudevents/sdk-javascript/tree/eca43d907468dd07d9f291bb56f95f658f4b3c40/examples/websocket. In order to implement the binding, i just edited the sample adding the logic for the subprotocols. On client side:

const ws = new WebSocket("ws://localhost:8080", ["json.cloudevents", "avro.cloudevents"]);

Which roughly translates to open a new websocket and tell the server that I support both json and avro serialized cloudevents.

On server side:

const wss = new WebSocket.Server({ 
  port: 8080,
  handleProtocols: (protocols, req) => {
    if (protocols.includes("application/cloudevents+json")) {
      return "application/cloudevents+json";
    }
    return false
  }
});

Which translates to tell the client I only support json and during the stream I'll always use json anyway.

The rest of the code is exactly the same as is now

| Subprotocol | Event format |
| ------------------ | -------------------------------- |
| `json.cloudevents` | [JSON event format][json-format] |
| `avro.cloudevents` | [AVRO event format][avro-format] |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at the W3C interface again, it appears that using a subprotocol that includes the event format is indeed the best option we have, even though I still find that iffy.

OneM2M seems to have established a precedent in the IANA registry. I would switch the names to "cloudevents.*". We should reach out to IANA and register at least "cloudevents.json", "cloudevents.avro", "cloudevents.protobuf", and "cloudevents.cbor".

What's missing for the subprotocol definitions is whether the subprotocol is using binary or text frames. I think we will want to default to use text frames for JSON, but binary frames for Avro and Proto.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's missing for the subprotocol definitions is whether the subprotocol is using binary or text frames. I think we will want to default to use text frames for JSON, but binary frames for Avro and Proto.

Should we define it in this table?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check it out now

## 3. WebSocket Message Mapping

Because the content mode is always _structured_, a WebSocket message just
contains a CloudEvent serialized using the agreed event format.
Copy link
Contributor

@tweing tweing Oct 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... WebSocket message just contains a CloudEvent ...

Is the JSON Batch Format supported as well? If yes, the sentence would need to be slightly adapted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a use case where you would send json batch over WS?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a use case and honestly I don't like it. Nevertheless since you are referring to the json format, it might be worth mentioning explicitly that it shall not be supported.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check it out now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine with me - no objections (sorry- can't make it for the meeting on 08-Oct-2020)

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Specified frame type

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
Copy link
Member Author

Rebased and fixed README

@duglin
Copy link
Collaborator

duglin commented Oct 8, 2020

Approved on the 10/8 call

@duglin duglin merged commit 6ffe844 into cloudevents:master Oct 8, 2020
@slinkydeveloper slinkydeveloper deleted the ws-protocol-binding branch October 8, 2020 18:35
n3wscott pushed a commit to n3wscott/cloudevents-spec that referenced this pull request Nov 30, 2020
* Draft of WebSockets protocol binding

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

* Fix

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

* Wrong wording

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

* Renamed subprotocols
Specified frame type

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

* Specify that json batch format is not supported

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

* Fix conflicts

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@duglin duglin mentioned this pull request Dec 7, 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

Successfully merging this pull request may close these issues.

WebSockets protocol binding
4 participants