Pass fetchOptions to fetch call to match HttpLink behavior #302
Conversation
|
@TrevinAvery: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
|
@TrevinAvery is this PR a duplicate of #296 ? -- If so, which one should I go with? Also, I can't proceed unless you sign the Apollo Contributor License agreement, please let me know i you want to continue! |
|
@fbartho I have signed the CLA. #296 does appear to add support for the abort controller, however, this PR supports all fetch options as overrides. I would take this one as it is more comprehensive. That said, I did not include a test that actually aborts a fetch and confirms it was cancelled. It may be worth at least pulling in that test from the other PR. |
src/__tests__/restLink.ts
Outdated
| @@ -2497,6 +2695,111 @@ describe('Query options', () => { | |||
| ]); | |||
| }); | |||
| }); | |||
| describe('body', () => { | |||
| it('prioritizes fetchOptions body over directive body', async () => { | |||
There was a problem hiding this comment.
Where does fetchOptions body come from? What is it for?
There was a problem hiding this comment.
There are a lot of interplaying features, and I'm not super comfortable with blindly accepting a body via a context parameter without understanding the spec.
The abortcontroller is interesting and makes sense to me, but I don't think the body should be passed through?
There was a problem hiding this comment.
I don't think the body option is necessarily the most useful thing, but I included it for completeness. I was copying the behavior in the HTTP Link, which simply passes all fetch options to the fetch function. I think it makes sense to include this option so no one tries to use it and gets confused by it not working (which is the state I was in when I tried to use an abort controller). It should not break any existing behavior.
There was a problem hiding this comment.
The problem is that we can't guarantee other Links aren't accidentally injecting a body, and if other links do inject a body, we can't parse the results or otherwise handle it.
I get why AbortController makes sense, but if we use the body, we're actually building apollo-link-rest into a really inefficient clone of HTTPLink that doesn't do the right thing?
Maybe I'm confused as to how this body is used normally and when it would be expected to be passed to this link.
|
I can't say for sure how likely it is that another link will pass in a body
and cause an issue. But I imagine if that happens, then it is unexpected,
and therefore a bug in how there links are configured. The body should
ideally be provided from only one source. But it seemed the existing
options were intended as overrides, so I matched that behavior. If this is
still a major concern, I see two options to resolve it:
1. Remove the body option and log a warning if one is received so the user
will know it is being ignored.
2. Add an option in the link constructor that enables or disables this (and
possibly other) fetch option.
…On Tue, Nov 15, 2022, 5:46 PM Frederic Barthelemy ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/__tests__/restLink.ts
<#302 (comment)>
:
> @@ -2497,6 +2695,111 @@ describe('Query options', () => {
]);
});
});
+ describe('body', () => {
+ it('prioritizes fetchOptions body over directive body', async () => {
The problem is that we can't guarantee other Links aren't accidentally
injecting a body, and if other links do inject a body, we can't parse the
results or otherwise handle it.
I get why AbortController makes sense, but if we use the body, we're
actually building apollo-link-rest into a really inefficient clone of
HTTPLink that doesn't do the right thing?
Maybe I'm confused as to how this body is used normally and when it would
be expected to be passed to this link.
—
Reply to this email directly, view it on GitHub
<#302 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGBCSFUGQ5UPRDGIHBSRMXDWIQ4GJANCNFSM6AAAAAARXVHT4A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I agree with you @TrevinAvery -- we don't know how likely it is for somebody to use it, but I bet it would initially be an indication of a bug. I'd be comfortable if you followed path 1, remove/disable the body option. And I'm fine if you log a warning. I don't think we should go with option 2, until and unless somebody specifically makes a case for this feature being very useful. -- Because then we can write tests and docs that explain the use! You also said:
And I agree here too, if you pull in that test, and make the change (1) you proposed, then I'd be inclined to build a new release that includes this feature. Just let me know! |
|
@fbartho Sorry this took so long. I have had some serious deadlines at work. I have made the changes we discussed. |
|
@fbartho Can you please review this PR and let me know if it is acceptable? |
|
@fbartho Are you still out there? Can we merge this in please? |
|
@fbartho Although this branch is now out of date, I still think it would be a valuable addition to the project. I would be happy to update the branch so it can be merged if there is any interest in adding this feature. Please let me know if we can move this forward, or if this pull request should instead be closed. |
|
hey @TrevinAvery - I missed that you had updated the commits in this PR after my last comment, and I haven’t been actively developing on this library so it hasn’t been top of mind. I’ll try to take a look soon! I generally am positive on the purpose of this change, just not sure what it’ll take to get this PR safe to merge! |
HttpLinkallowsfetchOptionsto be passed in thecontextof a query'soptions. This is then passed to thefetchcall. This allows for finer control of thefetch, such as attaching anAbortControllerso the it can be cancelled.This PR adds support for this same option to match this behavior. For values in
fetchOptionsthat can be passed in the link context or the query directive (method,headers,body, andcredentials), the values infetchOptionstake precedence.