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

fix(query-core): handle errors that occur inside setData method #7966

Merged
merged 12 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions packages/query-core/src/__tests__/query.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -965,4 +965,51 @@ describe('query', () => {
await sleep(60) // let it resolve
expect(spy).toHaveBeenCalledWith('1 - 2')
})

it('should have an error status when queryFn data is not serializable', async () => {
const consoleMock = vi.spyOn(console, 'error')

consoleMock.mockImplementation(() => undefined)

const key = queryKey()

const queryFn = vi.fn()

queryFn.mockImplementation(async () => {
await sleep(10)

const data: Array<{
id: number
name: string
link: null | { id: number; name: string; link: unknown }
}> = Array.from({ length: 5 })
.fill(null)
.map((_, index) => ({
id: index,
name: `name-${index}`,
link: null,
}))

if (data[0] && data[1]) {
data[0].link = data[1]
data[1].link = data[0]
}

return data
})

await queryClient.prefetchQuery({ queryKey: key, queryFn })

const query = queryCache.find({ queryKey: key })!

expect(queryFn).toHaveBeenCalledTimes(1)

expect(consoleMock).toHaveBeenCalledWith(
expect.stringContaining('Data is not serializable'),
)

expect(query.state.status).toBe('error')

consoleMock.mockRestore()
})
})
7 changes: 6 additions & 1 deletion packages/query-core/src/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,12 @@ export class Query<
return
}

this.setData(data)
try {
this.setData(data)
} catch (error) {
onError(error as TError)
return
}

// Notify cache callback
this.#cache.config.onSuccess?.(data, this as Query<any, any, any, any>)
Expand Down
13 changes: 13 additions & 0 deletions packages/query-core/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,19 @@ export function replaceData<
if (typeof options.structuralSharing === 'function') {
return options.structuralSharing(prevData, data) as TData
} else if (options.structuralSharing !== false) {
if (process.env.NODE_ENV !== 'production') {
try {
JSON.stringify(prevData)
JSON.stringify(data)

Choose a reason for hiding this comment

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

This now throws an error when data contains BigInt. But it seems like react-query supports having BigInt in the query result. Should I create an issue for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm think it's still good to log the error because structuralSharing creates an overhead and you should likely turn it off if you have something that isn't serializable. But we should likely remove the throw. Logging is enough. Please file a PR

Copy link

@ethos-vitalii ethos-vitalii Sep 2, 2024

Choose a reason for hiding this comment

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

Thank you @Efimenko for raising that PR 🙌

@TkDodo I have a question. From what I see in replaceEqualDeep, there's nothing leading BigInt comparison to break. For example, 123n === 123n will be true. Is there anything else I'm missing where BigInt could break this.setData()?

Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting. I didn't know that BigInt was referentially equal, but not json serializable. According to the documentation, we only support json serializable values in structuralSharing, so I think the warning is still warranted.

Copy link
Contributor

@jxom jxom Sep 6, 2024

Choose a reason for hiding this comment

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

Yeah, this seems to have affected a lot of upstream Wagmi users. Wonder if this should have been a breaking change. Changing from throwing to just a warning would be better though!

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's "just" a dev mode warning. Why would it be breaking?

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies. Didn't see this commit.

Copy link
Contributor

@jxom jxom Sep 7, 2024

Choose a reason for hiding this comment

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

@TkDodo – would you be open to a PR that checked for cyclic references instead of checking for non-serializable values via JSON.stringify? Feel like a lot of consumers would be invoking query functions that return BigInts, so it might be a bit odd seeing these warning messages flying around when replaceEqualDeep is also compatible with BigInts.

Wonder if it’s worth conveying that structural sharing is moreso only compatible with referentially stable values instead of JSON-serializable values.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wonder if it’s worth conveying that structural sharing is moreso only compatible with referentially stable values instead of JSON-serializable values.

I simply wasn't aware that replaceEqualDeep works with BigInt - our general recommendation is to use json serializable values. If we say BigInt is supported, then why isn't Map supported, or Set, or Date, or ... it's a slippery slope. Also, in queryKeys, we actually use JSON.stringify, so if you use a BigInt there, it will fail.

Why is JavaScript so weird that BigInt is referentially stable across values, but not serializable lol.

would you be open to a PR that checked for cyclic references instead of checking for non-serializable values via JSON.stringify?

yeah I guess that's fine. I think just calling replaceEqualDeep itself in a try/catch would just work, too ?

Copy link
Contributor

Choose a reason for hiding this comment

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

done! #8030

} catch (error) {
console.error(
`Data is not serializable. [${options.queryHash}]: ${error}; The error will be redacted in production builds`,
)

throw new Error('Data is not serializable')
}
}

// Structurally share data between prev and new data if needed
return replaceEqualDeep(prevData, data)
}
Expand Down
Loading