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.

Sign up for GitHub

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

fix(deps): make graphql an optional peer dependency #2442

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(deps): make graphql an optional peer dependency #2442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,16 +147,12 @@
"@mswjs/interceptors": "^0.37.0",
"@open-draft/deferred-promise": "^2.2.0",
"@open-draft/until": "^2.1.0",
"@types/cookie": "^0.6.0",
"@types/statuses": "^2.0.4",
"graphql": "^16.8.1",
"headers-polyfill": "^4.0.2",
"is-node-process": "^1.2.0",
"outvariant": "^1.4.3",
"path-to-regexp": "^6.3.0",
"picocolors": "^1.1.1",
"strict-event-emitter": "^0.5.1",
"type-fest": "^4.26.1",
"yargs": "^17.7.2"
},
"devDependencies": {
Expand All @@ -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",
Copy link
Member

Choose a reason for hiding this comment

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

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.

"@types/express": "^4.17.21",
"@types/fs-extra": "^11.0.4",
"@types/glob": "^8.1.0",
"@types/json-bigint": "^1.0.4",
"@types/node": "18.x",
"@types/statuses": "^2.0.4",
"@typescript-eslint/eslint-plugin": "^8.8.1",
"@typescript-eslint/parser": "^8.8.1",
"@web/dev-server": "^0.4.6",
Expand Down Expand Up @@ -201,6 +199,7 @@
"ts-node": "^10.9.2",
"tsup": "^8.3.0",
"typescript": "^5.5.2",
"type-fest": "^4.26.1",
"undici": "^6.20.0",
"url-loader": "^4.1.1",
"vitest": "^2.1.8",
Expand All @@ -209,10 +208,10 @@
"webpack-http-server": "^0.5.0"
},
"peerDependencies": {
"typescript": ">= 4.8.x"
Copy link
Member

Choose a reason for hiding this comment

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

We still need to keep TypeScript. I don't see how your proposal implies it has to be removed.

"graphql": ">=16.8.1"
Copy link
Member

Choose a reason for hiding this comment

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

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:

},
"peerDependenciesMeta": {
"typescript": {
"graphql": {
"optional": true
}
},
Expand Down
20 changes: 10 additions & 10 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.