-
Notifications
You must be signed in to change notification settings - Fork 2.9k
π€π€π€ perf: defer MissingFieldError construction and avoid JSON.stringify in cache diff #13329
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
π€π€π€ perf: defer MissingFieldError construction and avoid JSON.stringify in cache diff #13329
Changes from all commits
762fd5f
13d2b71
d096985
78c661e
a0e7652
0974d38
4bb6df7
1a7abbe
533e9c7
097b9c3
7bedb70
b578a7d
4358da5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@apollo/client": patch | ||
| --- | ||
|
|
||
| Cache diffs for incomplete queries no longer pay the cost of building a full `MissingFieldError` when the `missing` property is not accessed. The error object is now only constructed when the `missing` property is accessed the first time. This improves performance by avoiding a V8 stack capture when `missing` is ignored entirely. | ||
|
|
||
| As an additional small performance improvement, `JSON.stringify` is no longer used in the error message on objects whose cache ID is known. `JSON.stringify` is only used for non-normalized objects. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -224,18 +224,10 @@ export class StoreReader { | |
| }, | ||
| }); | ||
|
|
||
| let missing: MissingFieldError | undefined; | ||
| if (execResult.missing) { | ||
| missing = new MissingFieldError( | ||
| firstMissing(execResult.missing)!, | ||
| execResult.missing, | ||
| query, | ||
| variables | ||
| ); | ||
| } | ||
| const { result, missing: rawMissing } = execResult; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI apologies ahead of time. This code is going to change once I merge in my branch. Unfortunately I happened to be working on a bunch of tweaks to the cache reader at the same time. I'm only saying this so that if you go searching for these changes in |
||
| const complete = !rawMissing; | ||
|
|
||
| const complete = !missing; | ||
| const { result } = execResult; | ||
| let missingError: MissingFieldError | undefined; | ||
|
|
||
| return { | ||
| result: | ||
|
|
@@ -246,7 +238,17 @@ export class StoreReader { | |
| : result | ||
| : null, | ||
| complete, | ||
| missing, | ||
| get missing() { | ||
| if (missingError === void 0 && rawMissing) { | ||
| missingError = new MissingFieldError( | ||
| firstMissing(rawMissing)!, | ||
| rawMissing, | ||
| query, | ||
| variables | ||
| ); | ||
| } | ||
| return missingError; | ||
| }, | ||
| } as Cache.DiffResult<T>; | ||
| } | ||
|
|
||
|
|
@@ -338,11 +340,16 @@ export class StoreReader { | |
|
|
||
| if (fieldValue === void 0) { | ||
| if (!addTypenameToDocument.added(selection)) { | ||
| const id = | ||
| isReference(objectOrReference) ? objectOrReference.__ref | ||
| : objectOrReference ? policies.identify(objectOrReference)[0] | ||
| : undefined; | ||
|
|
||
| missing = missingMerger.merge(missing, { | ||
| [resultName]: `Can't find field '${selection.name.value}' on ${ | ||
| isReference(objectOrReference) ? | ||
| objectOrReference.__ref + " object" | ||
| : "object " + JSON.stringify(objectOrReference, null, 2) | ||
| id ? | ||
| `${id} object` | ||
| : `object ${JSON.stringify(objectOrReference || {}, null, 2)}` | ||
| }`, | ||
| }); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.