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

apollo-server-core: Remove inconsistent @cacheControl auto-definition #5182

Merged
merged 1 commit into from
May 6, 2021

Conversation

glasser
Copy link
Member

@glasser glasser commented May 5, 2021

In Apollo Server 2, ApolloServer sometimes adds the definition of
@cacheControl to your schema.

Specifically, it adds it whenever you rely on the implicit call to
graphql-tools' makeExecutableSchema (ie, you pass typeDefs and
resolvers). It doesn't add it when you pass your own schema, and
oddly it doesn't add it when you use modules.

On the other hand, it doesn't care if you actually have cache control
enabled in your app.

What this means is that switching between the implicit call to the
built-in version of makeExecutableSchema and using your own copy of
graphql-tools has a surprising impact on your schema. We'd like to
make the implicit call just a shorthand and encourage folks who want to
use all of the different options of makeExecutableSchema to call it
directly in their app (rather than having tons of pass-through
arguments), so we don't want the semantics to differ in a surprising and
undocumented way.

Plus, other tools that look at your schema will work better if you
define the @cacheControl directive explicitly.

AS2 did allow you to define the directive yourself (it only adds it if
it isn't there).

So this PR changes apollo-server-core to no longer auto-include the
definitions, and documents how to write them yourself.

(In the future, we may want to change @cacheControl to be a Core
Schema feature, but we're not doing that today.)

(AS2 had a similar issue with the types related to graphql-upload but
that integration is removed.)

Fixes #5181.

In Apollo Server 2, ApolloServer *sometimes* adds the definition of
`@cacheControl` to your schema.

Specifically, it adds it whenever you rely on the implicit call to
`graphql-tools`' `makeExecutableSchema` (ie, you pass `typeDefs` and
`resolvers`). It doesn't add it when you pass your own `schema`, and
oddly it doesn't add it when you use `modules`.

On the other hand, it doesn't care if you actually have cache control
enabled in your app.

What this means is that switching between the implicit call to the
built-in version of `makeExecutableSchema` and using your own copy of
`graphql-tools` has a surprising impact on your schema. We'd like to
make the implicit call just a shorthand and encourage folks who want to
use all of the different options of `makeExecutableSchema` to call it
directly in their app (rather than having tons of pass-through
arguments), so we don't want the semantics to differ in a surprising and
undocumented way.

Plus, other tools that look at your schema will work better if you
define the `@cacheControl` directive explicitly.

AS2 did allow you to define the directive yourself (it only adds it if
it isn't there).

So this PR changes `apollo-server-core` to no longer auto-include the
definitions, and documents how to write them yourself.

(In the future, we may want to change `@cacheControl` to be a Core
Schema feature, but we're not doing that today.)

(AS2 had a similar issue with the types related to `graphql-upload` but
that integration is removed.)

Fixes #5181.
@glasser glasser force-pushed the glasser/define-cachecontrol-yourself branch from b04b9bc to d43fc44 Compare May 5, 2021 23:08
@glasser
Copy link
Member Author

glasser commented May 5, 2021

FWIW it looks like the insertion was added as part of graphql 14 support in v2.0.6 at a point where changing to require the directives to be defined would be backwards-incompatible. I can't figure out from archaeology why modules, which post-dates v2.0.6, doesn't include the definitions.

@glasser
Copy link
Member Author

glasser commented May 6, 2021

On Slack, @abernix and @martijnwalraven signed off on the concept behind this PR; post-merge review of the implementation would be appreciated.

@glasser glasser merged commit 5a55a8b into release-3.0 May 6, 2021
@glasser glasser deleted the glasser/define-cachecontrol-yourself branch May 6, 2021 16:19
Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants