strawberry-graphql / strawberry

A GraphQL library for Python that leverages type annotations 🍓
https://strawberry.rocks
MIT License
3.87k stars 511 forks source link

Refactor graphql-ws message types #1669

Open DoctorJohn opened 2 years ago

DoctorJohn commented 2 years ago

Both the legacy graphql-ws and the newer graphql-transport-ws protocols are concerned with the processing of a handful of messages.

Our implementation of the newer protocol uses dataclasses to describe messages and their type. They are very pleasant to work with and look like this:

https://github.com/strawberry-graphql/strawberry/blob/b569bf79d5863731aeb7996f073c795616b473e5/strawberry/subscriptions/protocols/graphql_transport_ws/types.py#L17-L24

However, our implementation of the legacy protocol uses TypedDict to describe messages and separate constants for message types. In our case they are harder to work with.

We should refactor the legacy protocols message definitions and use dataclasses for them as well.

Upvote & Fund

Fund with Polar

DoctorJohn commented 1 year ago

Someone asked on Discord for clarification of the following statement:

In our case they are harder to work with.

Dataclasses allow us to store the message type within the dataclass. With typedicts we have to store the type of a message in a separate constant, creating an implicit dependency between the typedict and the separate constant. When working with graphql-ws messages we have to import both the typedicts and the constants. In this case the typedict-approach results in more imports and is more error prone due to the implicit dependencies between typedicts and constants.

patrick91 commented 1 year ago

@DoctorJohn I wonder when we should drop support for the legacy protocol 😊

DoctorJohn commented 1 year ago

@DoctorJohn I wonder when we should drop support for the legacy protocol 😊

Me too. The corresponding js library still has more than 2 million weekly downloads. So I assume it's still in use.