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

Use new Apollo type definitions #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jpumford
Copy link

@jpumford jpumford commented Oct 26, 2020

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
Copy link
Contributor

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants