fix(deps): make graphql an optional peer dependency #2442
Conversation
| @@ -166,11 +162,13 @@ | |||
| "@open-draft/test-server": "^0.4.2", | |||
| "@ossjs/release": "^0.8.1", | |||
| "@playwright/test": "^1.48.0", | |||
| "@types/cookie": "^0.6.0", | |||
There was a problem hiding this comment.
These types have to be shipped as regular dependencies. For libraries that are written in TS, their TS dependencies are regular dependencies, never dev. Otherwise they may not get installed and start causing issues for consumers.
| @@ -209,10 +208,10 @@ | |||
| "webpack-http-server": "^0.5.0" | |||
| }, | |||
| "peerDependencies": { | |||
| "typescript": ">= 4.8.x" | |||
There was a problem hiding this comment.
We still need to keep TypeScript. I don't see how your proposal implies it has to be removed.
| @@ -209,10 +208,10 @@ | |||
| "webpack-http-server": "^0.5.0" | |||
| }, | |||
| "peerDependencies": { | |||
| "typescript": ">= 4.8.x" | |||
| "graphql": ">=16.8.1" | |||
There was a problem hiding this comment.
Sadly, this isn't going to work. There isn't really a thing as optional peer dependency in JavaScript right now. You can think you defined one in peerDependencies, but that's just the installation part.
There's no way to consume an optional peer dependency in the library's code. Dynamic imports aren't supported everywhere, and requires cause issues with ESM.
We've tried this in the past to no avail, always meeting some edge case from our users.
Sharing some context in case you will be more successful than me:
|
Hi, @slinkardbrandon. Thanks for opening this. I left some comments on the code, elaborating on why this won't really work in practice. But the most important thing, perhaps, is this:
I strongly advise you configure your vulnerability report tooling to ignore dev dependencies, which is MSW (and its respective dev dependencies). Those do not pose any risk to your users and, by default, should be ignored. That will unblock you in your project. If you still want to pursue optional graphql, I'd be happy to help, but none of my attempts were successful (see the referenced issues). |
Working on an application that currently has vulnerabilities due to graphql but the application does not even use graphql, that eventually led us to this repo.
I updated the dependency chain so that graphql is an optional peer dependency which should enable apps to not install additional unwanted dependencies that are unused. Additionally, if you are working on an application with graphql, it probably makes sense to just use the host's graphql version anyways rather than having a duplicate from this library.
Edit: While in there I also moved some dependencies to dev deps that didn't need to be production level dependencies just to clean things up a bit.