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

refactor: Replace "cache-control" extension with plugin. #3997

Merged
merged 9 commits into from
May 12, 2020

Conversation

abernix
Copy link
Member

@abernix abernix commented Apr 16, 2020

Similar to 6009d8a (#3991), which migrated the tracing package to the plugin API, this commit introduces a plugin (named) export from the apollo-cache-control module. It similarly accomplishes that by duplicating the behavior of the CacheControlExtension which leveraged the soon-to-be-deprecated graphql-extensions. The functionality, again, is intended to be identical in spirit.

As with the afformentioned commit, since the delta of the commits was otherwise even more confusing, I've left the CacheControlExtension present and exported and will remove it in a subsequent commit.

Briefly summarizing what the necessary changes were:

  1. We no longer use a class instance to house the extension, which was necessitated by the graphql-extensions API. This means that uses of this have been replaced with function scoped variables by the same name.
  2. The logic which actually does the formatting (previously handled by the format method in graphql-extension, now takes place within the plugin API's willSendResponse method.
  3. Rather than leaning on plugin-specific behavior from within the request pipeline, the apollo-cache-control plugin now makes a number of assertions throughout its various life-cycle methods to ensure that the overallCachePolicy is calculated.
  4. Switch tests to use the new pluginTestHarness method for testing plugins which was introduced in eec87a6 (Introduce an internal plugin test harness to facilitate plugin testing. #3990).

…o it.

Similar to 6009d8a, which migrated the tracing package to the plugin
API, this commit introduces a `plugin` (named) export from the
`apollo-cache-control` module.  It similarly accomplishes that by
duplicating the behavior of the `CacheControlExtension` which leveraged the
soon-to-be-deprecated `graphql-extensions`.  The functionality, again, is
intended to be identical in spirit.

Since the delta of the commits was otherwise even _more_ confusing, I've
left the `CacheControlExtension` present and exported and will remove it in a
subsequent commit.

Briefly summarizing what the necessary changes were:

1. We no longer use a class instance to house the extension, which was
   necessitated by the `graphql-extensions` API.  This means that uses of
   `this` have been replaced with function scoped variables by the same name.
2. The logic which actually does the formatting (previously handled by the
   `format` method in `graphql-extension`, now takes place within the plugin
   API's `willSendResponse` method.
3. Rather than leaning on plugin-specific behavior from within the request
   pipeline, the `apollo-cache-control` plugin now makes a number of
   assertions throughout its various life-cycle methods to ensure that the
   `overallCachePolicy` is calculated.
4. Switch tests to use the new `pluginTestHarness` method for testing
   plugins which was introduced in eec87a6 (#3990).
Which has been replaced by the plugin API introduced in 68cbc93.
abernix added a commit that referenced this pull request Apr 16, 2020
…ns".

Similar to 6009d8a (#3991) and 68cbc93 (#3997), which migrated the
tracing and cache-control extensions to the (newer) request pipeline plugin
API, this commit introduces:

 - Internally, a `plugin` named export which is utilized by the `agent`'s
   `newExtension` method to provide a plugin which is instrumented to
   transmit metrics to Apollo Graph Manager.  This plugin is meant to
   replicate the behavior of the `EngineReportingExtension` class which,
   as of this commit, still lives besides it.
 - Externally, a `federatedPlugin` exported on the main module of the
   `apollo-engine-reporting` package.  This plugin is meant to replicate the
   behavior of the `EngineFederatedTracingExtension` class (also exported on
   the main module) which, again as of this commit, still lives besides it!

Again, the delta of the commits seemed more confusing by allowing a natural
`diff` to be made of it, I've left the extensions in place so they can be
compared - presumably side-by-side in an editor - on the same commit.  An
(immediate) subsequent commit will remove the extension.
@abernix abernix added 🔌 plugins Relates to the request pipeline plugin API 🧬 graphql-extensions labels Apr 16, 2020
@abernix abernix self-assigned this Apr 16, 2020
@abernix abernix added this to the Release 2.13.0 milestone Apr 16, 2020
@@ -790,7 +770,37 @@ export class ApolloServerBase {
// at the end of that list so they take precidence.
// A follow-up commit will actually introduce this.

// Enable cache control unless it was explicitly disabled.
if (this.config.cacheControl !== false) {
Copy link
Member

Choose a reason for hiding this comment

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

With cacheControl implemented as a plugin now, do you anticipate we'll remove this configuration point in a future major (v4)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly. I feel like our cacheControl integration might get tighter over time, but it's certainly possible we could have configuration be entirely composable via plugins. I don't imagine we'd sacrifice this particular convenience plugin initialization anytime soon though.

@abernix abernix modified the milestones: Release 2.13.0, Release 2.14.0 May 4, 2020
@abernix abernix changed the base branch from abernix/add-willResolveField-and-didResolveField to release-2.14.0 May 12, 2020 11:54
@abernix abernix merged commit 546865d into release-2.14.0 May 12, 2020
@abernix abernix deleted the abernix/migrate-cache-control-to-plugin-api branch May 12, 2020 12:17
},
}),

responseForOperation() {
Copy link
Member

Choose a reason for hiding this comment

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

@abernix Do you have any memory of why this is here? This runs before execution, so I don't see how the policy it calculates could ever be non-trivial. I don't see anything that obviously matches it on the "before" side of this PR. Tests pass without it (on my PR that refactors some stuff).

@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
🔌 plugins Relates to the request pipeline plugin API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants