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.

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

Parse incoming JSON asynchronously if possible #11207

Closed
wants to merge 7 commits into from

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Sep 11, 2023

Since we have support for multipart responses, we had to switch away from Response.json because we had to parse the response manually.
At least since it has been added to this repository, HttpLink always used Response.text() followed by JSON.parse. Unfortunately, that moved parsing to the main thread, and it became blocking, which can be problematic with very large responses.

This PR tries to move parsing off the main thread where possible. Queries that will have a multipart response are excluded from this - I had managed to make them async by creating a new Request object, followed by calling .json on that, but that would (after a short moment) still block the main thread and just make things slower.

The tests have been updated to have Response present in our test environment, so currently all of our tests are running this new behaviour.

We have to make a call with this PR: 89f99f9 adds quite a bit complexity. It's necessary to have the response body available in case of a ParsingError. We could decide to keep response off the error object in those cases, and work with the more simple logic I had in place before.

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 Sep 11, 2023

🦋 Changeset detected

Latest commit: 0155c66

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 Patch

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 Sep 11, 2023

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 37.35 KB (+0.2% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 43.89 KB (+0.33% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 42.46 KB (+0.34% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 32.66 KB (+0.43% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 31.43 KB (+0.51% 🔺)
import { ApolloProvider } from "dist/react/index.js" 1.21 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production) 1.2 KB (0%)
import { useQuery } from "dist/react/index.js" 4.27 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.08 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 4.58 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.4 KB (0%)
import { useMutation } from "dist/react/index.js" 2.52 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.51 KB (0%)
import { useSubscription } from "dist/react/index.js" 2.24 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 2.2 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 4.76 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production) 4.19 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 4.28 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.7 KB (0%)
import { useReadQuery } from "dist/react/index.js" 2.96 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production) 2.91 KB (0%)
import { useFragment } from "dist/react/index.js" 2.08 KB (0%)
import { useFragment } from "dist/react/index.js" (production) 2.03 KB (0%)

Comment on lines -207 to -214
if (response.status >= 300) {
// Network error
throwServerError(
response,
result,
`Response not successful: Received status code ${response.status}`
);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This has already been handled in the parseJsonBody method so we can remove it here.

@phryneas phryneas changed the base branch from main to release-3.9 September 12, 2023 12:53
@phryneas phryneas changed the base branch from release-3.9 to main September 12, 2023 12:56
@phryneas phryneas changed the base branch from main to release-3.9 September 12, 2023 12:56
@jerelmiller jerelmiller force-pushed the release-3.9 branch 2 times, most recently from c80ba36 to 3f7eecb Compare December 6, 2023 20:55
@phryneas
Copy link
Member Author

phryneas commented Jan 8, 2024

After further testing, it seems that no browser actually moves the parsing off-thread (although the spec would allow for it) and it will be blocking either way, so I will close this.
I'll open a new PR with some code deduplication I noticed during this PR.

@phryneas
Copy link
Member Author

phryneas commented Jan 8, 2024

Extracted that to #11470 - closing this.

@phryneas phryneas closed this Jan 8, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant