Add missing dependency to apollo-engine-reporting

[squirly]

A missing dependency was causing an issue where npm would inline the dependency that is required by this package in another package, making the dependency unavailable when requiring.

Used here

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

Line 16 in 6f5769c

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

[apollo-cla]

@squirly: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[abernix]

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?)

[squirly]

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.

[abernix]

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!

[abernix]

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

[squirly]

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.

[abernix]

Ah, this isn't about TypeScript types at all (not sure how I got hung up on that, but that's all I had focused my comments on!), but there's an actual runtime instantiation of of InMemoryLRUCache, as well!:

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

Line 532 in 31028e7

return new InMemoryLRUCache<string>({

Including the error with the full stack-trace can be really helpful in understanding these things, but thank you for the reproduction!

I've added a follow-up commit to this that updates the package-lock.json, and we'll get this in the next release.

Thanks!