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.

Sign up for GitHub

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

Add missing dependency to apollo-engine-reporting #3054

Merged
merged 5 commits into from
Jul 30, 2019
Merged

Add missing dependency to apollo-engine-reporting #3054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f92e9b5
Add missing dependency to `apollo-engine-reporting`
Jul 17, 2019
27336bf
Merge branch 'master' into patch-1
squirly Jul 29, 2019
4f37476
Merge branch 'master' into patch-1
squirly Jul 29, 2019
8fe8b78
Merge branch 'master' into HEAD
abernix Jul 30, 2019
f3717c7
Update `package-lock.json` for #3054.
abernix Jul 30, 2019
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
1 change: 1 addition & 0 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.

1 change: 1 addition & 0 deletions packages/apollo-engine-reporting/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"apollo-engine-reporting-protobuf": "file:../apollo-engine-reporting-protobuf",
"apollo-graphql": "^0.3.3",
"apollo-server-env": "file:../apollo-server-env",
"apollo-server-caching": "file:../apollo-server-caching",
Copy link
Member

Choose a reason for hiding this comment

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

Since it's a type that's only used within a private property on the exported EngineReportingAgent class, would this be better categorized within devDependencies?:

apollo-server/packages/apollo-engine-reporting/src/agent.ts

Lines 224 to 225 in 6f5769c

private signatureCache: InMemoryLRUCache<string>;

(i.e. Could you verify to see if having it as a dev-dep resolves the concern you identified?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not, as we are only use dependencies when packaging for deploy; and using devDependencies would not be correct per npm recommendations:

  • "dependencies": Packages required by your application in production.
  • "devDependencies": Packages that are only needed for local development and testing.

~ See Specifying dependencies and devDependencies in a package.json file.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I'm not saying that this dependency doesn't need to be added, but the rational for deciding whether types go into devDependencies or dependencies is not as straightforward as those npm recommendations you've listed.

Concretely, if we went by the npm standard that you're stating here — which I'm familiar with — we could arguably say that no types are necessary since normal dependencies since TypeScript is compiled to JavaScript before it runs in production and the type definitions are only used during development.

You're probably aware that typically, @types/* packages are installed as devDependencies, but I'm curious, did you test my suggestion or are you just trying to be educational? I'm not trying to be rude — and a reproduction of the problem or at least the error message would have made the solution more clear — but in the case of a library that provides typings (like apollo-engine-reporting, in this case!), it is important to put types into dependencies if those types appear in the emitted definition files (i.e. *.d.ts) since consuming packages need to be able to access those definitions during consuming application's compilation. However, in the case of InMemoryLRUCache within apollo-engine-reporting, again, its only used in a private property on an exported EngineReportingAgent class declaration.

That means that InMemoryLRUCache doesn't appear in the emitted types within agent.d.ts, which merely notes it as a private property.
Since it doesn't appear there, the InMemoryLRUCache type shouldn't be necessary.

(Btw, if it were not private, you're absolutely correct that it should be in dependencies, as it would be present in the agent.d.ts file!

Copy link
Member

Choose a reason for hiding this comment

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

Really, that's all to say, can you provide a reproduction or the error output? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Starting with the error:

Error: Cannot find module 'apollo-server-caching'

The stacktrace points to the transpiled version of:

apollo-server/packages/apollo-engine-reporting/src/agent.ts

Line 16 in 6f5769c

import { InMemoryLRUCache } from 'apollo-server-caching';

The issue is with how npm handles version incompatibilities when building a package-lock.

We are in a situation where this dependency is not inlined in the package-lock but the other dependency we use that requires apollo-server-caching is inlined not allowing this package to access the inlined version of apollo-server-caching.

Since npm does not install devDependencies in this case, adding to devDependencies does not cause the package to get installed in the correct place.

I do not want to include my entire package-lock.json so I created a paired down example based on it, https://gist.github.com/squirly/1452f6f1dbec894da41293b0b3713e06. It is contrived but exposes the issue.

If you run npm ci; then try to require this module, node -r apollo-server-caching, you will get the above error.

This example also exposed that graphql should be a dependency or peerDependency.

"apollo-server-types": "file:../apollo-server-types",
"async-retry": "^1.2.1",
"graphql-extensions": "file:../graphql-extensions"
Expand Down