-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Move cache declarations to @apollo/cache-control-types #6764
Conversation
🦋 Changeset detectedLatest commit: c1e4397 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for apollo-server-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c1e4397:
|
packages/server/package.json
Outdated
@@ -89,6 +89,7 @@ | |||
"node": ">=12.0" | |||
}, | |||
"dependencies": { | |||
"@apollo/cache-control-types": "^1.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually need to be a dependency in server? We only import types from it in runtime code. We use the functions in test code but that can still be a top-level dev dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, but I think the answer is yes, as the exported type GraphQLRequestContext
has an overallCachePolicy: CachePolicy
field.
However, I think the point holds for plugin-response-cache, so I'll remove it from there.
A quick pass from @rkoron007 on the docs change would be good too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small tweaks for style, but otherwise looks good to me!
(This package temporarily resides in the apollo-utils repo so that we can publish it as non-alpha during the AS4 alpha process, eg for use in `@apollo/subgraph`.) Notably, this removes the `declare module` declaration. This does mean that TypeScript users will have to work a tad harder (as shown in the migration guide) to access dynamic cacheControl, but it also means that we no longer have to worry about multiple copies of `declare module` in a TypeScript build conflicting with each other, which has been a serious source of pain in this past.
d0035cb
to
663af80
Compare
Co-authored-by: Rose M Koron <32436232+rkoron007@users.noreply.github.com>
Until now, the refactored AS4 did not support Apollo Gateway (or any implementation of the AS3 `gateway` option). That's because `GraphQLRequestContext` is part of the API between Apollo Gateway and Apollo Server, and that type has changed in some minor but incompatible ways in AS4. (Additionally, clashes between `declare module` declarations in AS3 and AS4 caused issue, but we removed those declarations from this branch in PRs #6764 and #6759.) This commit restores gateway support. It does this by having AS4 produce an AS3-style request context object. It uses a new `@apollo/server-gateway-interface` package to define the appropriate types for connecting to Gateway. (Note: this package will be code reviewed in this PR in this repo, but once it's approved, it will move to the apollo-utils repo and be released as non-alpha there. Once we've released AS4.0.0 we can move that package back here, but trying to do some prereleases and some non-prereleases on the same branch seems challenging.) This PR removes the top-level `executor` function, which is redundant with the `gateway` option. (Internally, the relevant field is now named `gatewayExecutor`.) Some types had been parametrized by `TContext`, because in AS3, `GraphQLExecutor` (now `GatewayExecutor`) appeared to take a `<TContext>`. However, even though the type itself took a generic argument, its main use in the return from `gateway.load` implicitly hardcoded the default `TContext`. So we are doubling down on that and only allowing `GraphQLExecutor` to use AS3's default `TContext`, the quite flexible `Record<string, any>`. Most of the way toward #6719.
Until now, the refactored AS4 did not support Apollo Gateway (or any implementation of the AS3 `gateway` option). That's because `GraphQLRequestContext` is part of the API between Apollo Gateway and Apollo Server, and that type has changed in some minor but incompatible ways in AS4. (Additionally, clashes between `declare module` declarations in AS3 and AS4 caused issue, but we removed those declarations from this branch in PRs #6764 and #6759.) This commit restores gateway support. It does this by having AS4 produce an AS3-style request context object. It uses a new `@apollo/server-gateway-interface` package to define the appropriate types for connecting to Gateway. (Note: this package will be code reviewed in this PR in this repo, but once it's approved, it will move to the apollo-utils repo and be released as non-alpha there. Once we've released AS4.0.0 we can move that package back here, but trying to do some prereleases and some non-prereleases on the same branch seems challenging.) This PR removes the top-level `executor` function, which is redundant with the `gateway` option. (Internally, the relevant field is now named `gatewayExecutor`.) Some types had been parametrized by `TContext`, because in AS3, `GraphQLExecutor` (now `GatewayExecutor`) appeared to take a `<TContext>`. However, even though the type itself took a generic argument, its main use in the return from `gateway.load` implicitly hardcoded the default `TContext`. So we are doubling down on that and only allowing `GraphQLExecutor` to use AS3's default `TContext`, the quite flexible `Record<string, any>`. Most of the way toward #6719. Co-authored-by: Trevor Scheer <trevor.scheer@gmail.com>
(This package temporarily resides in the apollo-utils repo so that we
can publish it as non-alpha during the AS4 alpha process, eg for use in
@apollo/subgraph
.)Notably, this removes the
declare module
declaration. This does meanthat TypeScript users will have to work a tad harder (as shown in the
migration guide) to access dynamic cacheControl, but it also means that
we no longer have to worry about multiple copies of
declare module
ina TypeScript build conflicting with each other, which has been a serious
source of pain in this past.