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

Conversation

acao
Copy link
Member

@acao acao commented Jun 1, 2021

must lock to graphql-ws@4.1.3 for now
load static modules script is not compatible with any later version.
I get ERR_PACKAGE_PATH_NOT_EXPORTED?

must lock to graphql-ws@4.1.3 for now
load static modules script is not compatible with any later version.
I get ERR_PACKAGE_PATH_NOT_EXPORTED
@acao acao changed the title chore: upgrade dependencies chore: upgrade devdependencies, specify optionalDependencies Jun 1, 2021
@acao acao changed the title chore: upgrade devdependencies, specify optionalDependencies chore: upgrade devDependencies, specify optionalDependencies Jun 1, 2021
@acao
Copy link
Member Author

acao commented Jun 1, 2021

@kyarik if you want to give this a review I'd love to have the help!

"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. 👍

"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 graphql-ws@4.1.3, 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?

package.json Outdated
Comment on lines 114 to 115
"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

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

@acao
Copy link
Member Author

acao commented Jul 7, 2021

We still need to remove graphiql-subscriptions-fetcher and a few other unused modules

@acao
Copy link
Member Author

acao commented Jul 9, 2021

@kyarik addressed your feedback and I also switched to using v1 by default, since v0 is a vulnerable path technically. this puts us under 100% coverage because we don't even want graphql-subscriptions-ws in the lockfile for the same reason. we can eventually create a separate package.json for legacy integration tests, but for now i've lowered the threshold. codecov doesn't like this but oh well

@acao
Copy link
Member Author

acao commented Jul 9, 2021

ugh. these integration tests are a nice idea but they seem to be causing a lot of issues... not sure when this was added

https://github.com/graphql/express-graphql/pull/764/checks?check_run_id=3027687126

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants