Proposal to change the API #4
Conversation
Currently the GraphQLDataSource package kind of follows RESTDataSource, but also uses ApolloLink infrastructure. It'd be great to have this basically like using ApolloClient, e.g.
class MyApi extends GraphQLDataSource {
constructor() {
this.link = ApolloLink.from([
onError,
createHttpLink({
fetch,
uri: "https://swapi/graphql"
})
])
super();
}
fetchStarships(type) {
this.query(gql`...`, ...);
}
}
This would allow custom error handling, usage of batching, retries, etc.
This commit also adds some missing types and raises several questions.
| import { DocumentNode } from 'graphql'; | ||
| import fetch from 'isomorphic-fetch'; |
There was a problem hiding this comment.
Oh, and I've made this change because people may want to bring their own fetch() (e.g., if you're using this in a serverless or Cloudflare-worker environment, you will already have a fetch method available.
| private async execute(operation: GraphQLRequest) { | ||
| try { | ||
| return await makePromise(execute(this.link, operation)); | ||
| } catch (error) { |
There was a problem hiding this comment.
using try/catch instead of await-to-js lightens the dependencies and keeps the code still readable
|
|
||
| private async execute(operation: GraphQLRequest) { | ||
| try { | ||
| return await makePromise(execute(this.link, operation)); |
There was a problem hiding this comment.
Ideally once we go towards caching, we could do something like what Apollo Client does here: https://github.com/apollographql/apollo-client/blob/master/packages/apollo-client/src/core/QueryManager.ts#L265-L307
| if (this.willSendRequest) { | ||
| this.willSendRequest(request); | ||
| await this.willSendRequest(request); |
There was a problem hiding this comment.
async this.willSendRequest to be inline with the protocol/types of DataSource and RESTDataSource
| } | ||
|
|
||
| return request; | ||
| }); | ||
| } | ||
|
|
||
| private onErrorLink() { |
There was a problem hiding this comment.
This should probably be handled by the user, as each user is like to have different error reporting requirements.
|
|
||
| protected willSendRequest?(request: any): any; | ||
|
|
||
| private composeLinks(): ApolloLink { |
There was a problem hiding this comment.
no need to do this every request, as the ApolloLink doesn't persist any state, so we can do it once per construction, rather than every request.
| } | ||
|
|
||
| public async mutation(mutation: DocumentNode, options: GraphQLRequest) { | ||
| // GraphQL request requires the DocumentNode property to be named query | ||
| return this.executeSingleOperation({ ...options, query: mutation }); | ||
| return this.execute({ ...options, query: mutation }); |
There was a problem hiding this comment.
taking request from Apollo Client's code
| type ValueOrPromise<T> = T | Promise<T>; | ||
| type RequestOptions = Record<string, any>; | ||
|
|
||
| export class GraphQLDataSource<TContext = any> extends DataSource { |
There was a problem hiding this comment.
extending DataSource for inheritance and compatibility
|
|
||
| private resolveUri(): string { | ||
| const baseURL = this.baseURL; | ||
| protected willSendRequest?(request: RequestOptions): ValueOrPromise<void>; |
There was a problem hiding this comment.
I think these are the correct Type signatures.
| const baseURL = this.baseURL; | ||
| protected willSendRequest?(request: RequestOptions): ValueOrPromise<void>; | ||
|
|
||
| private async execute(operation: GraphQLRequest) { |
There was a problem hiding this comment.
We probably need an operation build, ala, Apollo Client, where for example we can auto-set a default operationName — https://github.com/apollographql/apollo-client/blob/master/packages/apollo-client/src/core/QueryManager.ts#L1255-L1284
operationName: getOperationName(document) || undefined,
|
@ThisIsMissEm - Hey! Thank you for this, I apologize for the delay. I will review your proposed changes this week, at first glance I'm super stoked to see what you have done! |
|
@ThisIsMissEm @kmills006 can we make this happen? This is the first/only graphql datasource for ApolloServer - would love to contribute to this instead of rolling my own DataSource. I think the proposed changes are great and would love to see them merged. Then add resolveURL() and some tests. It all depends on if this PR get's merged. |
| return response; | ||
| } | ||
|
|
||
| private resolveUri(): string { |
There was a problem hiding this comment.
please don't remove this per https://www.apollographql.com/docs/apollo-server/v2/features/data-sources.html#Resolving-URLs-dynamically.
Should be renamed to resolveURL
|
Hi! I'm no longer using this in a work context, so I'm not sure when I'll get time to loopback around and update it. Feel free to make changes.
Emelia
… On 21. Dec 2018, at 15:15, Henrik Kok Jørgensen ***@***.***> wrote:
@hkjorgensen commented on this pull request.
In src/GraphQLDataSource.ts:
> }
- private async executeSingleOperation(operation: GraphQLRequest) {
- const link = this.composeLinks();
-
- const [error, response] = await to(makePromise(execute(link, operation)));
-
- if (error) {
- this.didEncounterError(error);
- }
-
- return response;
- }
-
- private resolveUri(): string {
please don't remove this per https://www.apollographql.com/docs/apollo-server/v2/features/data-sources.html#Resolving-URLs-dynamically.
Should be renamed to resolveURL
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Novice programmer here, made a package to use datasource pattern for graphql endpoints with Apollo Client cache for stitching, would love comments/suggestions on the approach, the code, whatever, etc, etc. Documentation: https://yaacovcr.github.io/apollo-stitcher I am not too familiar with TypeScript, much of this could probably be reoriented by those more in the know as pull requests to graphql-tools and/or this package. |
|
@kmills006 These changes seem like they would really benefit the community and set the stage for more people to be able to contribute in the right direction. Given that the author isn't in a position to merge, could you finish the effort? |
Currently the GraphQLDataSource package kind of follows RESTDataSource, but also uses ApolloLink infrastructure. It'd be great to have this basically like using ApolloClient, e.g.
This would allow custom error handling, usage of batching, retries, etc.
This commit also adds some missing types and raises several questions.