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.

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

apollo-datasource-rest should declare graphql as peerDependency #6392

Closed
Grmiade opened this issue May 5, 2022 · 4 comments · Fixed by #6393
Closed

apollo-datasource-rest should declare graphql as peerDependency #6392

Grmiade opened this issue May 5, 2022 · 4 comments · Fixed by #6393

Comments

@Grmiade
Copy link

Grmiade commented May 5, 2022

apollo-datasource-rest needs apollo-server-errors which has graphql as peerDependency.
So apollo-datasource-rest should also declare graphql as peerDependency in order to not breaking the chain.

@glasser
Copy link
Member

glasser commented May 5, 2022

This one I don't understand. a-d-r does not use graphql (it actually has literally nothing to do with GraphQL, it's just a caching HTTP client). It does correctly depend on apollo-server-errors which correctly peer deps on graphql. Why would everything that depends on apollo-server-errors also need to peer dep on graphql?

@Grmiade
Copy link
Author

Grmiade commented May 5, 2022

When we depend of a package with a peer dependency we have to declare it as dependency.
Here apollo-datasource-rest depends of apollo-server-errors with graphql as peer dependency.
But apollo-datasource-rest doesn't need graphql because it doesn't use it. So instead of declare it as dependency we should declare it as peer dependency, in order to indicate to the packages which use apollo-datasource-rest they should install graphql, not for apollo-datasource-rest but for apollo-server-errors, it's what I called the "chain".

PnP being more strict with this kind of stuff, this kind of errors are raised:

Error: apollo-server-errors tried to access graphql (a peer dependency) but it isn't provided by its ancestors; this makes the require call ambiguous and unsound.

Required package: graphql
Required by: apollo-server-errors@virtual:1a27f047ab26746f7fed08c476ddf4561a87794f226019306e917a0a4ea8babfbb3fbc3a9852e50021103b37e9a00084a79a6cb0373b52642ae7377b88868c26#npm:3.3.1 (via /usr/src/app/.yarn/__virtual__/apollo-server-errors-virtual-cf59f2326d/0/cache/apollo-server-errors-npm-3.3.1-f3e7ce6ccc-d09b66e7ac.zip/node_modules/apollo-server-errors/dist/)

Ancestor breaking the chain: apollo-datasource-rest@npm:3.5.3
Require stack:
- /usr/src/app/.yarn/__virtual__/apollo-server-errors-virtual-cf59f2326d/0/cache/apollo-server-errors-npm-3.3.1-f3e7ce6ccc-d09b66e7ac.zip/node_modules/apollo-server-errors/dist/index.js
- /usr/src/app/.yarn/cache/apollo-datasource-rest-npm-3.5.3-1a27f047ab-faed1fb8b1.zip/node_modules/apollo-datasource-rest/dist/RESTDataSource.js
- /usr/src/app/.yarn/cache/apollo-datasource-rest-npm-3.5.3-1a27f047ab-faed1fb8b1.zip/node_modules/apollo-datasource-rest/dist/index.js

@glasser
Copy link
Member

glasser commented May 5, 2022

Ah hmm. I suppose that makes sense, although it's a bit strange.

glasser added a commit that referenced this issue May 5, 2022
Some packages were depending on other packages that were only declared
as transitive dependencies. Clean this up by adding appropriate
dependencies (or in one case, just re-declaring ValueOrPromise and
dropping the apollo-server-types dependency).

The peer dep one is a bit funny.  But "Y has a peer dep on Z" means
"when you install Y you need to install Z", and so if X depends on Y,
then when you install X you need to install Z... so sure, that means X
needs to have a peer dep on Z too, I guess.

Fixes #6389.
Fixes #6390.
Fixes #6391.
Fixes #6392.
glasser added a commit that referenced this issue May 5, 2022
Some packages were depending on other packages that were only declared
as transitive dependencies. Clean this up by adding appropriate
dependencies (or in one case, just re-declaring ValueOrPromise and
dropping the apollo-server-types dependency).

The peer dep one is a bit funny.  But "Y has a peer dep on Z" means
"when you install Y you need to install Z", and so if X depends on Y,
then when you install X you need to install Z... so sure, that means X
needs to have a peer dep on Z too, I guess.

Fixes #6389.
Fixes #6390.
Fixes #6391.
Fixes #6392.
glasser added a commit that referenced this issue May 5, 2022
Some packages were depending on other packages that were only declared
as transitive dependencies. Clean this up by adding appropriate
dependencies (or in one case, just re-declaring ValueOrPromise and
dropping the apollo-server-types dependency).

The peer dep one is a bit funny.  But "Y has a peer dep on Z" means
"when you install Y you need to install Z", and so if X depends on Y,
then when you install X you need to install Z... so sure, that means X
needs to have a peer dep on Z too, I guess.

Fixes #6389.
Fixes #6390.
Fixes #6391.
Fixes #6392.
@Grmiade
Copy link
Author

Grmiade commented May 6, 2022

Thanks a lot @glasser for the 4 issues 💪

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
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 a pull request may close this issue.

2 participants