chore: dependency update for subscriptions-transport-ws

[Maxwell2022]

Is there an existing issue for this?

• I have searched the existing issues

Current behavior

I've searched for it and this issue has been historically closed here: #2406

This project is now archived and flag as deprecated (as of April 14th 2023)

From official Apollo v4 documentation

This article uses the graphql-ws library to add support for subscriptions to Apollo Server 4. We no longer recommend using the previously documented subscriptions-transport-ws, because this library is not actively maintained.

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?

• macOS
• Windows
• Linux

Other

No response

[erikwrede]

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.

[Maxwell2022]

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 use graphql-ws instead. For help migrating Apollo software to graphql-ws, see https://www.apollographql.com/docs/apollo-server/data/subscriptions/#switching-from-subscriptions-transport-ws For general help using graphql-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

[erikwrede]

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.

[ssipos90]

shouldn't the packages be peer dependencies, both subscription-transport-ws and graphql-ws?

[belmeopmenieuwesim]

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.

[ssipos90]

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

[elijaholmos]

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.

[ruscon]

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.

[ctretyak]

A lot of users don't use ws, but have this warn while npm install

[Jarinha99]

Another problem with keeping subscriptions-transport-ws is that it has old dependencies that have vulnerabilities, as long as we keep subscriptions-transport-ws, we will continue to install libraries with vulnerabilities in our projects.

Example:
(ws "^5.2.0 || ^6.0.0 || ^7.0.0")
websockets/ws#2230

[iacobus]

If you encounter this issue because of the vulnerable ws version introduced by subscriptions-transport-ws, we were able to address it in our project by forcing a patch version upgrade (ws backported the security fix to major versions 5/6/7). Using yarn, you may add a resolutions field to your package.json, listing the dependency paths that lead to a vulnerable version, e.g.:

  "resolutions": {
    "**/subscriptions-transport-ws/ws": "7.5.10",
    "**/webpack-dev-server/ws": "8.17.1"
  }

[onichandame]

@ruscon Would you mind sharing you setup if possible? I just tried nestjs + yoga + graphql-sse but the subscription requests all failed with code 415. The normal @Sse endpoints in controller work fine tho.

[ImNicolasTheDev]

As I can see, this discussion is still open.

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.

I agree with that, since some users would still like to use the old deprecated dep subscriptions-transport-ws. However, most users see a warning and must install a deprecated dependency...

Even though @iacobus mentionned a patch could be applied to fix vulnerabilities issues, I think most users would prefer to use it with the new graphql-ws dep instead of the old one.

What do you think about releasing a major version without the deprecated subscriptions-transport-ws ? Users could still use previous versions, but could choose to use whatever version they want / need / prefer 😄