search

apollographql / apollo-link-rest

Use existing REST endpoints with GraphQL
MIT License
791 stars 122 forks source link

Pass fetchOptions to fetch call to match HttpLink behavior #302

Open TrevinAvery opened 2 years ago

TrevinAvery commented 2 years ago

HttpLink allows fetchOptions to be passed in the context of a query's options. This is then passed to the fetch call. This allows for finer control of the fetch, such as attaching an AbortController so the it can be cancelled.

This PR adds support for this same option to match this behavior. For values in fetchOptions that can be passed in the link context or the query directive (method, headers, body, and credentials), the values in fetchOptions take precedence.

apollo-cla commented 2 years ago

@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/

fbartho commented 1 year ago

@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!

TrevinAvery commented 1 year ago

@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.

TrevinAvery commented 1 year ago

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 https://github.com/apollographql/apollo-link-rest/pull/302#discussion_r1023421115 :

@@ -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 https://github.com/apollographql/apollo-link-rest/pull/302#discussion_r1023421115, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGBCSFUGQ5UPRDGIHBSRMXDWIQ4GJANCNFSM6AAAAAARXVHT4A . You are receiving this because you were mentioned.Message ID: @.***>

fbartho commented 1 year ago

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:

It may be worth at least pulling in that test from the other PR.

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!

TrevinAvery commented 1 year ago

@fbartho Sorry this took so long. I have had some serious deadlines at work. I have made the changes we discussed.

TrevinAvery commented 1 year ago

@fbartho Can you please review this PR and let me know if it is acceptable?

TrevinAvery commented 1 year ago

@fbartho Are you still out there? Can we merge this in please?

TrevinAvery commented 7 months ago

@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.

fbartho commented 7 months ago

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!