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

[Bug] yarn pnpify tsc can not resolve some graphql types #271

Closed
mohsen1 opened this issue Jul 1, 2019 · 8 comments
Closed

[Bug] yarn pnpify tsc can not resolve some graphql types #271

mohsen1 opened this issue Jul 1, 2019 · 8 comments
Labels
bug Something isn't working

Comments

@mohsen1
Copy link

mohsen1 commented Jul 1, 2019

Describe the bug

See my PR trying to use P'n'P on this simple project that uses TypeScript and Webpack:

mohsen1/apollo-react-async-ssr#3

I get errors like the following.

note

What's significant about Apollo packages is that they ship their own types.

.yarn/cache/apollo-server-plugin-base-npm-0.5.6-cc277148df89efcb60033ea282a14f7c47f09911109e741cf6c7c6ccce45bc57.zip/node_modules/apollo-server-plugin-base/dist/index.d.ts:1:95 - error TS2307: Cannot find module 'apollo-server-core/dist/requestPipelineAPI'.

1 import { GraphQLServiceContext, GraphQLRequestContext, GraphQLRequest, GraphQLResponse } from 'apollo-server-core/dist/requestPipelineAPI';
                                                                                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.yarn/cache/graphql-extensions-npm-0.7.4-c0f20210a7443737d662bd6d508e284683a78027a2816cb42cade84f293591da.zip/node_modules/graphql-extensions/dist/index.d.ts:2:25 - error TS2307: Cannot find module 'apollo-server-env'.

2 import { Request } from 'apollo-server-env';
                          ~~~~~~~~~~~~~~~~~~~

Environment:

  • OS: Linux
  • Node version 10
  • Yarn version 2

Additional context

tsc does pass with the code in master.

@mohsen1 mohsen1 added the bug Something isn't working label Jul 1, 2019
@arcanis
Copy link
Member

arcanis commented Jul 1, 2019

I believe this is a apollo-server-plugin-base problem - they only depend on apollo-server-core as a dev dependency, so the imported types cannot be found. Cf here:

https://cdn.jsdelivr.net/npm/apollo-server-plugin-base@0.5.6/package.json

@mohsen1
Copy link
Author

mohsen1 commented Jul 1, 2019

How this was working before? 🤔

@arcanis
Copy link
Member

arcanis commented Jul 1, 2019

My guess is that some other package depended on apollo-server-core and, by the magic of the hoisting, apollo-server-plugin-base was stealing it. PnP enforces proper package boundaries (a package can only access its dependencies), so situations like this can't happen.

@JacksonKearl
Copy link

apollo-server-plugin-base only depends on apollo-server-core for typings (makes sense, as the entire apollo-server-plugin-base is just typings), and as such does not require apollo-server-core outside of a dev environment. I am not very familiar with standard procedure when it comes to type-only packages, what would be appropriate here?

@mohsen1
Copy link
Author

mohsen1 commented Jul 2, 2019

It is advised that packages never depend to their @types/* packages as a direct or a peer dependency because of various issues that sort of dependency causes.

Since Apollo packages don't require a type dependency it's a bit different. I am not sure what is the right thing to do here.

Installing apollo-server-core as a direct dependency in my TypeScript compiled package will solve my issue. But this is a bad DX. It took a while to find this out.

Maybe TypeScript should have a concept of "type dependencies" and Yarn can install those if TypeScript is involved...

/cc @DanielRosenwasser

mohsen1 added a commit to mohsen1/apollo-react-async-ssr that referenced this issue Jul 2, 2019
@arcanis
Copy link
Member

arcanis commented Jul 2, 2019

IMO, generally speaking, the @types packages should list peer dependencies, not regular dependencies. This has been my stance for quite some time:

In the particular case of Apollo, it seems like things should work better with using a regular dependency (because unless I'm mistaken it doesn't export global symbols; if it did then a peer dependency would be safer).


Maybe TypeScript should have a concept of "type dependencies" and Yarn can install those if TypeScript is involved...

We have an experimental TypeScript plugin that would be the perfect ground for such things (right now it simply auto-adds the corresponding @types packages when you add something else). Of course it wouldn't work with npm 🤷‍♀️

@arcanis
Copy link
Member

arcanis commented Aug 7, 2019

I'll close this issue as it seems caused by an improper dependency listing in the Apollo package.

@arcanis arcanis closed this as completed Aug 7, 2019
@CallMeLaNN
Copy link

CallMeLaNN commented Aug 30, 2019

@arcanis Does this related to apollographql/apollo-server#3222 ?

My theory is that pnpify can't resolve @types/* of child packages that being use by package maintainer. I just created this repo to reproduce the error.

apollo-server-express > @types/express > @types/express-serve-static-core

I do have the first two because I'm using it. Since Yarn flattened all dependencies on the tree, I also have .yarn/cache/@types-express-serve-static-core...zip. but I have to add @types/express-serve-static-core into my devDep too in order to update .pnp.js and make it work.

Update: apollo-server-express avoid that by explicitly use type exported by @types/express

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants