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

Spec: Added reason to WebSocket Disconnect #462

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

frankie567
Copy link
Contributor

This is a proposal to fix the discussion in #459

Clients can send this reason field when disconnecting. This is allowed as per RFC 6455 and supported in browsers JS API, but previously not supported in ASGI.


I took the liberty to bump the spec version, but I can remove it if you prefer 🙂

@@ -189,6 +189,7 @@ class WebSocketResponseBodyEvent(TypedDict):
class WebSocketDisconnectEvent(TypedDict):
type: Literal["websocket.disconnect"]
code: int
reason: Optional[str]
Copy link
Contributor

Choose a reason for hiding this comment

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

@andrewgodwin Can we start using NotRequired on those?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately that only appears in 3.11...

Copy link
Contributor

Choose a reason for hiding this comment

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

asgiref depends on typing_extensions, so it's already possible.

@@ -22,6 +22,7 @@ This spec has had the following versions:
* ``2.2``: Allow ``None`` in the second item of ``server`` scope value.
* ``2.3``: Added the ``reason`` key to the WebSocket close event.
* ``2.4``: Calling ``send()`` on a closed connection should raise an error
* ``2.5``: Added the ``reason`` key to the WebSocket disconnect event.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need a bump on the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not for me to say, but I saw it was done similarly for 2.3. Can remove it if not needed :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, any spec change should minor bump if it's backwards-compatible (like this) and a major bump if it is not.

Copy link
Member

Choose a reason for hiding this comment

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

You did miss out updating the main version number though - we'll need that bumped too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You did miss out updating the main version number though - we'll need that bumped too.

Done 👍

@frankie567
Copy link
Contributor Author

Anything I can do so we can go forward with this? 🙂

@@ -22,6 +22,7 @@ This spec has had the following versions:
* ``2.2``: Allow ``None`` in the second item of ``server`` scope value.
* ``2.3``: Added the ``reason`` key to the WebSocket close event.
* ``2.4``: Calling ``send()`` on a closed connection should raise an error
* ``2.5``: Added the ``reason`` key to the WebSocket disconnect event.
Copy link
Member

Choose a reason for hiding this comment

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

You did miss out updating the main version number though - we'll need that bumped too.

Clients can send this reason field when disconnecting. This is allowed as per RFC 6455 and supported in browsers JS API, but previously not supported in
ASGI.

Fix django#459
@andrewgodwin andrewgodwin merged commit f86642b into django:main Jun 12, 2024
6 checks passed
@andrewgodwin
Copy link
Member

Thanks for your help on this!

@Kludex
Copy link
Contributor

Kludex commented Sep 28, 2024

@andrewgodwin can you push an update to the docs, please?

@andrewgodwin
Copy link
Member

It was meant to auto-build but it seems the webhook became unhappy - just initiated a manual build, so it should be good soon.

@Kludex
Copy link
Contributor

Kludex commented Oct 1, 2024

It was meant to auto-build but it seems the webhook became unhappy - just initiated a manual build, so it should be good soon.

Thanks!

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.

3 participants