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.

Sign up for GitHub

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

Jump to bottom

(Batch)HttpLink: only swallow internal AbortError #11058

Merged
merged 5 commits into from
Jul 13, 2023
Merged

(Batch)HttpLink: only swallow internal AbortError #11058

merged 5 commits into from
Jul 13, 2023

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Jul 12, 2023 •
edited
Loading

Fixes #10640

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@changeset-bot
Copy link

changeset-bot bot commented Jul 12, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 4bf588a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Jul 12, 2023 •
edited
Loading

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 37.06 KB (-0.05% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 43.66 KB (-0.03% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 42.26 KB (-0.03% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 32.39 KB (-0.04% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 31.22 KB (-0.05% 🔽)
import { ApolloProvider } from "dist/react/index.js" 1.21 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production) 1.19 KB (0%)
import { useQuery } from "dist/react/index.js" 4.25 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.08 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 4.57 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.39 KB (0%)
import { useMutation } from "dist/react/index.js" 2.49 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.47 KB (0%)
import { useSubscription } from "dist/react/index.js" 2.23 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 2.19 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 4.69 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production) 4.12 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 4.17 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.6 KB (0%)
import { useReadQuery } from "dist/react/index.js" 2.66 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production) 2.63 KB (0%)
import { useFragment } from "dist/react/index.js" 2.04 KB (0%)
import { useFragment } from "dist/react/index.js" (production) 1.99 KB (0%)

phryneas
phryneas commented Jul 12, 2023
View reviewed changes
src/link/batch-http/batchHttpLink.ts Outdated
@@ -180,7 +181,7 @@ export class BatchHttpLink extends ApolloLink {
.catch(err => {
controller = undefined;
// fetch was cancelled so its already been cleaned up in the unsubscribe
if (err.name === 'AbortError') return;
if (err.name === 'AbortError' && internalAbortSignal?.aborted) return;
Copy link
Member Author

@phryneas phryneas Jul 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

To be honest, I am not even sure if this line is necessary at all.
It was introduced back in 2017 with apollographql/apollo-link@c66b1da#diff-ab3906d63911de4980d8d24d3e314fd448eced2ead8ace4bb1d3ecb9b44bedad .

I cannot reproduce any situation where it would be necessary, but at can also not guarantee that this is always unneccesary, so I'm adding this additional check.

phryneas
phryneas commented Jul 12, 2023
View reviewed changes
src/link/http/__tests__/HttpLink.ts Outdated
Comment on lines 1415 to 1427
it('aborting the internal signal will not cause an error', async () => {
try {
fetchMock.restore();
fetchMock.postOnce('data', async () => '{ "data": { "stub": { "id": "foo" } } }');
const abortControllers = trackGlobalAbortControllers();

const link = createHttpLink({ uri: '/data' });
execute(link, { query: sampleQuery } ).subscribe(failingObserver);
abortControllers[0].abort();
} finally {
fetchMock.restore();
}
});
Copy link
Member Author

Choose a reason for hiding this comment

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

I could not find a situation where this would normally be the case - the AbortController will only be aborted if the subsciption is unsubscribed, and any errors after that should not have any consequence anyways.
This is the closest test I could create to trigger a situation where the .abort() call could have any consequence, and that's already testing implementation details.

@phryneas phryneas requested review from jerelmiller and alessbell and removed request for jerelmiller July 12, 2023 13:36
@phryneas
remove check completely
5560491 
since it would not have any consequence when triggered by internal `.abort()` calls.
@phryneas
Copy link
Member Author

phryneas commented Jul 13, 2023 •
edited
Loading

Based on my comments above ( here and here ) and my discussion with Jerel about it, this PR now removes the check completely.

@phryneas phryneas self-assigned this Jul 13, 2023
@jerelmiller
Copy link
Member

@phryneas love it!

jerelmiller
jerelmiller approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

🎉 Great change! Love the recognition that the conditional is not needed anymore.

.changeset/polite-pianos-kiss.md Outdated Show resolved Hide resolved
@jerelmiller jerelmiller linked an issue Jul 13, 2023 that may be closed by this pull request
Calling AbortController abort causes mutation promise to never resolve #10640
Closed
@jerelmiller jerelmiller merged commit 89bf33c into release-3.8 Jul 13, 2023
4 checks passed
@jerelmiller jerelmiller deleted the pr/only-catch-internal-AbortError branch July 13, 2023 17:58
@github-actions github-actions bot mentioned this pull request Jul 13, 2023
Merged
@jerelmiller jerelmiller mentioned this pull request Jul 13, 2023
Closed
@jerelmiller
Copy link
Member

This should fix #9583 as well.

@jerelmiller jerelmiller mentioned this pull request Jul 19, 2023
Closed
This was referenced Aug 2, 2023
Closed
Merged
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jerelmiller jerelmiller jerelmiller approved these changes

@alessbell alessbell Awaiting requested review from alessbell

Assignees

@phryneas phryneas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Calling AbortController abort causes mutation promise to never resolve
2 participants
@phryneas @jerelmiller