chore: upgrade devDependencies, specify optionalDependencies #764
Conversation
must lock to [email protected] for now load static modules script is not compatible with any later version. I get ERR_PACKAGE_PATH_NOT_EXPORTED
acao
changed the title
acao
changed the title
|
@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", |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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?
package.json
Outdated
| "graphql-ws": "4.1.3", | ||
| "graphiql-subscriptions-fetcher": "0.0.2" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point, they should be specified in the readme instead
|
We still need to remove |
|
@kyarik addressed your feedback and I also switched to using |
|
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 |
must lock to
[email protected]for nowload static modules script is not compatible with any later version.
I get
ERR_PACKAGE_PATH_NOT_EXPORTED?