-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Bail out of query update if data is undefined or same as previous data #2905
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
85d1795
d509752
9142d09
0e5f85f
282f511
5868036
21fa1ec
9e77be1
65f8676
7d15c84
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 |
---|---|---|
|
@@ -102,6 +102,10 @@ interface SuccessAction<TData> { | |
dataUpdatedAt?: number | ||
} | ||
|
||
interface IgnoreAction { | ||
type: 'ignore' | ||
} | ||
|
||
interface ErrorAction<TError> { | ||
type: 'error' | ||
error: TError | ||
|
@@ -134,6 +138,7 @@ export type Action<TData, TError> = | |
| PauseAction | ||
| SetStateAction<TData, TError> | ||
| SuccessAction<TData> | ||
| IgnoreAction | ||
|
||
export interface SetStateOptions { | ||
meta?: any | ||
|
@@ -220,28 +225,37 @@ export class Query< | |
} | ||
|
||
setData( | ||
updater: Updater<TData | undefined, TData>, | ||
updater: Updater<TData | undefined, TData | undefined>, | ||
options?: SetDataOptions | ||
): TData { | ||
): TData | undefined { | ||
const prevData = this.state.data | ||
|
||
// Get the new data | ||
let data = functionalUpdate(updater, prevData) | ||
|
||
// Use prev data if an isDataEqual function is defined and returns `true` | ||
if (this.options.isDataEqual?.(prevData, data)) { | ||
data = prevData as TData | ||
} else if (this.options.structuralSharing !== false) { | ||
// Structurally share data between prev and new data if needed | ||
data = replaceEqualDeep(prevData, data) | ||
if (typeof data !== 'undefined') { | ||
// Use prev data if an isDataEqual function is defined and returns `true` | ||
if (this.options.isDataEqual?.(prevData, data)) { | ||
data = prevData | ||
} else if (this.options.structuralSharing !== false) { | ||
// Structurally share data between prev and new data if needed | ||
data = replaceEqualDeep(prevData, data) | ||
} | ||
} | ||
|
||
// Set data and mark it as cached | ||
this.dispatch({ | ||
data, | ||
type: 'success', | ||
dataUpdatedAt: options?.updatedAt, | ||
}) | ||
if (typeof data === 'undefined' || Object.is(data, prevData)) { | ||
// Bail out | ||
this.dispatch({ | ||
type: 'ignore', | ||
}) | ||
} else { | ||
// Set data and mark it as cached | ||
this.dispatch({ | ||
data, | ||
type: 'success', | ||
dataUpdatedAt: options?.updatedAt, | ||
}) | ||
} | ||
|
||
return data | ||
} | ||
|
@@ -366,6 +380,8 @@ export class Query< | |
options?: QueryOptions<TQueryFnData, TError, TData, TQueryKey>, | ||
fetchOptions?: FetchOptions | ||
): Promise<TData> { | ||
const prevData = this.state.data | ||
|
||
if (this.state.isFetching) { | ||
if (this.state.dataUpdatedAt && fetchOptions?.cancelRefetch) { | ||
// Silently cancel current fetch if the user wants to cancel refetches | ||
|
@@ -450,10 +466,12 @@ export class Query< | |
fn: context.fetchFn as () => TData, | ||
abort: abortController?.abort?.bind(abortController), | ||
onSuccess: data => { | ||
this.setData(data as TData) | ||
const updatedData = this.setData(data as TData) | ||
|
||
// Notify cache callback | ||
this.cache.config.onSuccess?.(data, this as Query<any, any, any, any>) | ||
if (typeof updatedData !== 'undefined' && !Object.is(prevData, updatedData)) { | ||
// Notify cache callback | ||
this.cache.config.onSuccess?.(updatedData, this as Query<any, any, any, any>) | ||
} | ||
|
||
// Remove query after fetching if cache time is 0 | ||
if (this.cacheTime === 0) { | ||
|
@@ -588,6 +606,16 @@ export class Query< | |
isPaused: false, | ||
status: 'success', | ||
} | ||
case 'ignore': | ||
return { | ||
...state, | ||
error: null, | ||
fetchFailureCount: 0, | ||
isFetching: false, | ||
isInvalidated: false, | ||
isPaused: false, | ||
status: typeof state.data === 'undefined' ? 'idle' : 'success', | ||
} | ||
Comment on lines
+609
to
+618
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. why do we need this please? Can we just not dispatch anything in case of bail-out and let the query be in that state as if 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. If a query function returns undefined or the same data, we need to reset state after bailing out. Otherwise the query will stay in fetching state. |
||
case 'error': | ||
const error = action.error as unknown | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,7 +68,7 @@ export class QueryObserver< | |
TQueryKey | ||
> | ||
private previousQueryResult?: QueryObserverResult<TData, TError> | ||
private previousSelectError: Error | null | ||
private previousSelectError: TError | null | ||
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. this change seems unrelated. Why is it necessary please? 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. These are type errors in the latest version of Typescript. I can fix in separate PR if you prefer. 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. yes lets please do this in a different branch, where we also update our TS version to the latest (4.5 as of a couple of minutes ago) |
||
private staleTimeoutId?: number | ||
private refetchIntervalId?: number | ||
private currentRefetchInterval?: number | false | ||
|
@@ -495,8 +495,8 @@ export class QueryObserver< | |
this.previousSelectError = null | ||
} catch (selectError) { | ||
getLogger().error(selectError) | ||
error = selectError | ||
this.previousSelectError = selectError | ||
error = selectError as TError | ||
this.previousSelectError = selectError as TError | ||
errorUpdatedAt = Date.now() | ||
status = 'error' | ||
} | ||
|
@@ -538,8 +538,8 @@ export class QueryObserver< | |
this.previousSelectError = null | ||
} catch (selectError) { | ||
getLogger().error(selectError) | ||
error = selectError | ||
this.previousSelectError = selectError | ||
error = selectError as TError | ||
this.previousSelectError = selectError as TError | ||
errorUpdatedAt = Date.now() | ||
status = 'error' | ||
} | ||
|
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.
I think we would a polyfill for that, please see: https://github.com/facebook/react/blob/cae635054e17a6f107a39d328649137b83f25972/packages/shared/objectIs.js
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.
ignore this if we go the route suggested in a later comment: #2905 (comment)
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.
I will add polyfill.