Repository navigation
Skip unchanged cached subtrees when removing nested nulls #856
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
Changes from all commits
41d640a
6839ba1
6ee53a2
7aecfc0
b6722ef
94edaee
65f5f9d
ffe0a02
0293e8a
7b045dd
8a86770
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 | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||
| import {deepEqual} from 'fast-equals'; | ||||||
| import type {OnyxInput, OnyxKey} from './types'; | ||||||
| import * as Logger from './Logger'; | ||||||
|
|
||||||
| type EmptyObject = Record<string, never>; | ||||||
| type EmptyValue = EmptyObject | null | undefined; | ||||||
|
|
@@ -176,7 +177,8 @@ function mergeObject<TObject extends Record<string, unknown>>( | |||||
| // of the merged object only. | ||||||
| const sourcePropertyWithoutMark = {...sourceProperty}; | ||||||
| delete sourcePropertyWithoutMark.ONYX_INTERNALS__REPLACE_OBJECT_MARK; | ||||||
| destination[key] = sourcePropertyWithoutMark; | ||||||
| // Built in "mark" mode, so it can still hold nested nulls. | ||||||
| destination[key] = options.shouldRemoveNestedNulls ? removeNestedNullValues(sourcePropertyWithoutMark) : sourcePropertyWithoutMark; | ||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -264,12 +266,25 @@ function needsNormalization(value: unknown): boolean { | |||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| /** Deep removes the nested null values from the given value. Returns the original reference if no nulls were found. */ | ||||||
| function removeNestedNullValues<TValue extends OnyxInput<OnyxKey> | null>(value: TValue): TValue { | ||||||
| /** | ||||||
| * Deep removes nested null values from the given value, leaving arrays as they are. Returns the original reference if no nulls were found. | ||||||
| * | ||||||
| * Objects that are the same reference as the object at the same path in `nullFreeReference` are returned without being traversed, | ||||||
| * so `nullFreeReference` must hold no nested nulls outside arrays (pass the live cache value). | ||||||
| */ | ||||||
|
Comment on lines
+269
to
+274
Contributor
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. NAB: arrays are never cleaned (neither
Contributor
Author
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. Good point! updated the JSDoc to say arrays are left as they are |
||||||
| function removeNestedNullValues<TValue extends OnyxInput<OnyxKey> | null>(value: TValue, nullFreeReference?: unknown): TValue { | ||||||
| if (value === null || value === undefined || typeof value !== 'object' || Array.isArray(value)) { | ||||||
| return value; | ||||||
| } | ||||||
|
|
||||||
| if (value === nullFreeReference) { | ||||||
| if (process.env.NODE_ENV === 'development' && needsNormalization(value)) { | ||||||
|
Contributor
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.
Suggested change
We should log during tests too, no?
Contributor
Author
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. It's kept dev-only on purpose, in tests the logger is a no-op, so the alert wouldn't show up anywhere unless a test checks for it, and there's already a dedicated test that does - Reassure also runs with NODE_ENV=test, so the extra check on every skipped subtree would change the perf CI numbers |
||||||
| Logger.logAlert('removeNestedNullValues skipped a cached subtree that holds nested null values. Onyx values must not be mutated in place.'); | ||||||
| } | ||||||
| return value; | ||||||
| } | ||||||
|
|
||||||
| const reference = nullFreeReference !== null && typeof nullFreeReference === 'object' && !Array.isArray(nullFreeReference) ? (nullFreeReference as Record<string, unknown>) : undefined; | ||||||
| let hasChanged = false; | ||||||
| const result: Record<string, unknown> = {}; | ||||||
|
|
||||||
|
|
@@ -283,7 +298,7 @@ function removeNestedNullValues<TValue extends OnyxInput<OnyxKey> | null>(value: | |||||
| } | ||||||
|
|
||||||
| if (typeof propertyValue === 'object' && !Array.isArray(propertyValue)) { | ||||||
| const cleaned = removeNestedNullValues(propertyValue); | ||||||
| const cleaned = removeNestedNullValues(propertyValue, reference?.[key]); | ||||||
| if (cleaned !== propertyValue) { | ||||||
| hasChanged = true; | ||||||
| } | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: No test covers this fix. If this line is changed back to c
onst value = valueFromStorage;, all tests still pass.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test for it, thanks!