Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

chore: upgrade devDependencies, specify optionalDependencies #764

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

chore: upgrade devDependencies, specify optionalDependencies #764

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
213bc20
chore: upgrade dependencies
acao Jun 1, 2021
d14e8c2
chore: fix ws docs, make graphql-ws v1 default, remove extraneous deps
acao Jul 9, 2021
2f61394
chore: temporarily disable require_ci_to_pass for codecov
acao Jul 9, 2021
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
119 changes: 92 additions & 27 deletions package-lock.json
View file
Open in desktop

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

16 changes: 10 additions & 6 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@
"express": "4.17.1",
"graphiql": "^1.4.1",
"graphiql-subscriptions-fetcher": "0.0.2",
"graphql": "15.4.0",
"graphql-ws": "4.1.2",
"graphql": "15.5.0",
"graphql-ws": "4.1.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding locking to [email protected], we could bypass the ERR_PACKAGE_PATH_NOT_EXPORTED by not using require.resolve, but instead do a plain path concat:

- const filePath = require.resolve(npmPath);
+ const filePath = path.join(process.cwd(), 'node_modules', npmPath);

WDYT?

"mocha": "8.2.1",
"multer": "1.4.2",
"nyc": "15.1.0",
Expand All @@ -102,12 +102,16 @@
"sinon": "9.2.1",
"subscriptions-transport-ws": "0.9.18",
"supertest": "6.0.1",
"ts-node": "9.0.0",
"typescript": "4.1.2",
"ts-node": "10.0.0",
"typescript": "4.3.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we upgraded TypeScript, I think we should also upgrade @typescript-eslint/eslint-plugin and @typescript-eslint/parser to the latest version (4.26.0) to avoid the warning:

WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.

At the same time, it would make sense to upgrade eslint to the latest 7.27.0. 👍

"unfetch": "4.2.0",
"ws": "5.2.2"
"ws": "7.4.6"
},
"peerDependencies": {
"graphql": "^14.7.0 || ^15.3.0"
"graphql": "^14.7.0 || ^15.5.0"
},
"optionalDependencies": {
"graphql-ws": "4.1.3",
"graphiql-subscriptions-fetcher": "0.0.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we listing these as optional dependencies? 🤔
It is unlikely that npm will fail to install them, so they will almost always be installed for the users of express-graphql (unless we expect users to run npm install --no-optional).
However, for those not using GraphQL subscriptions, these are two unnecessary dependencies. On the other hand, for those using subscriptions, it should be up to them to install these dependencies.

Copy link
Member Author

@acao acao Jul 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, they should be specified in the readme instead

}
}