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: Add option to specify fetcher for schema reporting #5179

Merged
merged 6 commits into from May 19, 2021

Conversation

sachindshinde
Copy link
Contributor

@sachindshinde sachindshinde commented May 5, 2021

We've gotten feedback that it would be nice for customers to have the ability to override the HTTP client for schema reporting. This PR adds that ability under a new experimental_fetcher option in ApolloServerPluginSchemaReporting (the syntax is similar to GatewayConfigBase.fetcher). Since there's not significant logic changes involved, we may also want to just forgo marking it experimental, although I'll leave that decision to the reviewer.

@sachindshinde sachindshinde requested a review from glasser May 5, 2021 21:29
Copy link
Member

@glasser glasser left a comment

Choose a reason for hiding this comment

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

This seems reasonable.

I don't think it needs to be experimental. fetcher would follow the pattern we use in two places in federation (I think the only other pattern we have in this repo for it involves RESTDataSource which takes it as an unnamed argument).

Would you like to apply the same change the usage reporting plugin? Not sure why people would only want to override the client for one.

@@ -2,6 +2,7 @@ import os from 'os';
import type { InternalApolloServerPlugin } from '../internalPlugin';
import { v4 as uuidv4 } from 'uuid';
import { printSchema, validateSchema, buildSchema } from 'graphql';
import { fetch } from 'apollo-server-env';
Copy link
Member

Choose a reason for hiding this comment

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

this could be import type? (We didn't use import type in the past because we wanted to support older TS but have started using it recently.)

@sachindshinde sachindshinde force-pushed the sachin/add-fetcher-for-schema-reporting branch from ab8bd85 to baa4b55 Compare May 19, 2021 07:16
@sachindshinde
Copy link
Contributor Author

@glasser
Sounds good, I've removed the experimental_ prefix from the option and also added a fetcher option to the usage reporting plugin (also updated the CHANGELOG.md).

@glasser glasser enabled auto-merge (squash) May 19, 2021 17:38
@glasser glasser merged commit d6b1bd4 into main May 19, 2021
@glasser glasser deleted the sachin/add-fetcher-for-schema-reporting branch May 19, 2021 17:41
@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.

None yet

2 participants