search

zapier / apollo-server-integration-testing

Test helper for writing apollo-server integration tests
MIT License
133 stars 21 forks source link

Use new Apollo type definitions #11

Open jpumford opened 4 years ago

jpumford commented 4 years ago

Since I was having the same issue as in #6, I made a quick fix locally and linked it. Seems to work just fine, but this is a pretty big change in the dependency tree, especially since the new apollo-server-express types were incompatible with this project's TS version which necessitated an upgrade.

I'm not sure if the type is backwards-compatible so since it sounds like Zapier is using an older version of apollo-server-express this could be a breaking change. It's a shame to have to lock step with apollo-server-express but I can't think of a reasonable other way around this.

EDIT: a lot of the diff is just integrity checks added by the newer version of yarn.

vitorbal commented 4 years ago

Hey @jpumford thanks for the PR! I think we might be able to circumvent this type issues entirely if we declare apollo-server-express as a peer dependency instead 🤔 I'm going to see if I can find sometime to dig into this during this week, and to give your PR a spin as well!

jpumford commented 4 years ago

Not a problem @vitorbal! I considered making the switch to a peer dependency, but I was worried about some of the weirdness that I thought could happen with semver matching:

(I am using imaginary versions) Suppose apollo-server-integration-testing requires apollo-server-express with peer dependency version ^1.3.0. Imagine that locally, apollo-server-integration-testing uses 1.4.5, which matches ^1.3.0 in the semver sense. This means apollo-server-integration-testing may have access to new properties on the ApolloServer type that don't exist in apollo-server-express 1.3.0, but do exist on version 1.4.5. TS allows us to use these properties since we are using 1.4.5 in our yarn.lock file.

Now suppose someone installs apollo-server-integration-testing, and already has apollo-server-express 1.3.0 in their package.json. No warnings are issued since 1.3.0 fulfills the peer dependency requirement, but errors are encountered since we are using features from 1.4.0.

That situation can be fixed if we bump the peer dependency requirement appropriately, but requires human intervention and would be hard to detect if a mistake was made. So in a sense I think the peer dependency option is more brittle for this repo's maintainers, but more flexible for people who are on newer versions of apollo-server-express.

At the end of the day, I can't make that decision for this repo, but if my reasoning is sound and the tradeoff seems acceptable I'd be happy to update this PR.