Update RESTDataSource and HTTPCache for Apollo Server v4
#1
Conversation
| } | ||
|
|
||
| export interface RequestWithBody extends Omit<RequestOptions, 'body'> { | ||
| method?: 'POST' | 'PUT' | 'PATCH' | 'DELETE'; |
There was a problem hiding this comment.
I don't think DELETE should allow a body
There was a problem hiding this comment.
Looks like it's actually legit-ish https://stackoverflow.com/questions/299628/is-an-entity-body-allowed-for-an-http-delete-request
| @@ -89,13 +86,14 @@ export abstract class RESTDataSource<TContext = any> extends DataSource { | |||
| } | |||
|
|
|||
| protected cacheOptionsFor?( | |||
| response: Response, | |||
| request: Request, | |||
| url: string, | |||
There was a problem hiding this comment.
Why is this sometimes a string and sometimes an URL?
There was a problem hiding this comment.
Let me know what you think: bed450e
I think this mostly comes down to convenience and signature matching. As things get closer to the Fetcher, using a string is more natural.
There was a problem hiding this comment.
Sure. Just thinking that for protected (overridable) methods it makes sense to be consistent rather than minimalist.
src/RESTDataSource.ts
Outdated
| ...request, | ||
| body: undefined, | ||
| }; | ||
| if (!(modifiedRequest.params instanceof URLSearchParams)) { |
There was a problem hiding this comment.
These two things maybe could use a comment saying "for convenience, make sure that the request always has params and headers so it's easy to add to it" or something (if that's the point?)
There was a problem hiding this comment.
Yep. This took me down a little rabbit hole, curious what you think.
b25c880
src/RESTDataSource.ts
Outdated
|
|
||
| protected resolveURL(request: RequestOptions): ValueOrPromise<URL> { | ||
| let path = request.path; | ||
| protected resolveURL(path: string): ValueOrPromise<URL> { |
There was a problem hiding this comment.
This is protected so maybe keeping the full RequestOptions is a good idea
There was a problem hiding this comment.
Are you looking to not break the existing API here or just ensure the same information that was previously available continue to be? path isn't on RequestOptions anymore. I can merge to make a single object (preserve existing API), which I've kind of avoided doing throughout / in other places.
Here's what I have (which just ensures previous information continues to be available)
352c298
There was a problem hiding this comment.
The latter (for subclasses), so that change looks good.
| type URLSearchParamsInit = ConstructorParameters<typeof URLSearchParams>[0]; | ||
|
|
||
| export type RequestOptions = FetcherRequestInit & { | ||
| params?: URLSearchParamsInit; |
There was a problem hiding this comment.
just to be clear: this is not "a feature of Fetch that we haven't bothered to mention in Fetcher", but a feature of RESTDataSource itself, right?
It's a bit weird to mix "things that come from the Fetch spec" and "our own stuff" but I guess that's fine.
There was a problem hiding this comment.
That's correct, though kind of surprising as I look more closely.
It looks like the fetch API allows body to be a URLSearchParams?
https://developer.mozilla.org/en-US/docs/Web/API/Request#examples
Maybe I'm going about this wrong by creating a distinction? The two types that I originally split (GetRequest and RequestWithBody) would theoretically collapse to just the one type if I treated search params as a body.
Idk, that seems a little weird and overloaded to me.
There was a problem hiding this comment.
I believe that literally does put the URLSearchParams in the body, with an appropriate content-type of application/x-www-form-urlencoded. https://fetch.spec.whatwg.org/#bodyinit-unions
This is actually the thing that POST
s do by default! https://developer.mozilla.org/en-US/docs/Web/HTML/Element/form#attr-enctypeso not really relevant to controlling the actual URL params.
fix: add ability to skip checking http cache
Update the
RESTDataSourceclass to be compatible with Apollo Server v4's new approach to datasources. This means initialization happens in the constructor (there's no more magic call toinitializedone by Apollo Server).Migrate to our
Fetcherinterface (and related types). This is a bit "restrictive" in that it doesn't accept theRequestclass fromnode-fetch, but the signature should be more cross compatible with other fetch implementation.Remove
apollo-server-envcompletely, update tests to use nock.Fixes apollographql/apollo-server#6048