Open Maxwell2022 opened 1 year ago
I disagree with this one. Just dropping protocol support because the protocol is not maintained anymore doesn't do graphql a favor. This library has full support for graphql-ws
, but there may be users relying on subscriptions-transport-ws
. Cutting them off from updates to nestjs/graphql
doesn't feel like the right way to do it. Especially because both protocols are working perfectly fine. It's just recommended for clients to use the newer library and protocol. The server should continue supporting both in the foreseeable future.
Just dropping protocol support because the protocol is not maintained anymore doesn't do graphql a favor
It's not dropping protocols 🤔. From my understanding websocket is still supported but using graphql-ws
.
but there may be users relying on subscriptions-transport-ws
These users could migrate to graphql-ws
as recommended by the maintainer of subscriptions-transport-ws
The
subscriptions-transport-ws
package is no longer maintained. We recommend you usegraphql-ws
instead. For help migrating Apollo software tographql-ws
, see https://www.apollographql.com/docs/apollo-server/data/subscriptions/#switching-from-subscriptions-transport-ws For general help usinggraphql-ws
, see https://github.com/enisdenjo/graphql-ws/blob/master/README.md
You don't want to drag old packages that could introduce security risk
It's not dropping protocols 🤔. From my understanding websocket is still supported but using graphql-ws.
There are two separate GraphQL protocols on top of web sockets that determine the client server communication messages. One of them (the older one) is subscriptions-transport-ws
, the other, newer one is graphql-ws
. They are incompatible with each other and a client has to support each protocol separately.
Since it's just a protocol (more explicitly an agreement on the structure of GraphQL Data and connection initialization) on top of Websocket, I don't really see a security risk here.
However, I agree about using old, archived dependencies. The alternative would be to implement the subscription-transport-ws
protocol inside of @nestjs/graphql
without the external library. For now, all of the 5 dependencies of transport-ws
seem to be up to date though https://github.com/apollographql/subscriptions-transport-ws/blob/master/package.json.
I'd say as long as Apollo & server keep supporting the old protocol there should be no reason to drop support. Some older apps and clients don't support graphql-ws
yet, and switching to the new protocol would cut them off of updates for nestjs.
shouldn't the packages be peer dependencies, both subscription-transport-ws and graphql-ws?
This package should 100% not be forced. Instead like @ssipos90 already said, it should be a peer dependency. Would not break anything, just require users to install the optional subscriptions-transport-ws
package.
I checked and it's also required to be added as an optional peer dependency: https://docs.npmjs.com/cli/v7/configuring-npm/package-json#peerdependenciesmeta
cc @belmeopmenieuwesim
This is what semantic versioning is for. Release a major version with a breaking change that switches from subscriptions-transport-ws
to graphql-ws
. Users are not forced to adopt the newest @nestjs/graphql
version if they do not want to switch WS clients; likewise, other users are not forced to use a package which has a deprecated dependency if they do not want to.
In our project we are currently using nestjs
+ graphql
+ subscriptions via sse
(yoga
+ http 2), so need to think about whether ws
is needed in this case.
A lot of users don't use ws, but have this warn while npm install
Is there an existing issue for this?
Current behavior
I've searched for it and this issue has been historically closed here: https://github.com/nestjs/graphql/issues/2406
This project is now archived and flag as deprecated (as of April 14th 2023)
From official Apollo v4 documentation
Minimum reproduction code
https://github.com/nestjs/graphql/blob/master/packages/graphql/package.json#L31
Steps to reproduce
No response
Expected behavior
this package should not used deprecated package but the recommended one
Package version
11
Graphql version
graphql
:apollo-server-express
:apollo-server-fastify
:NestJS version
No response
Node.js version
18
In which operating systems have you tested?
Other
No response