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.

Sign up for GitHub

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

Jump to bottom

Escape hatch added to defaultSerializeQueryArgs for unserializable data #4297

Closed
wants to merge 6 commits into from
Closed

Escape hatch added to defaultSerializeQueryArgs for unserializable data #4297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
60fcfd4
added test for BigInt arg handling
riqts Mar 23, 2024
0d1ae86
added suite for SerializeArgs hooks tests
riqts Mar 23, 2024
7764777
Escape hatch for unserializable values
riqts Mar 23, 2024
7095971
Removed redundant initializer
riqts Mar 23, 2024
3cdff56
removed comment
riqts Mar 23, 2024
2639cea
altered union ordering for consistency
riqts Mar 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/toolkit/src/query/defaultSerializeQueryArgs.ts
View file
Open in desktop
Expand Up @@ -10,14 +10,16 @@ export const defaultSerializeQueryArgs: SerializeQueryArgs<any> = ({
endpointName,
queryArgs,
}) => {
let serialized = ''
let serialized

const cached = cache?.get(queryArgs)

if (typeof cached === 'string') {
serialized = cached
} else {
const stringified = JSON.stringify(queryArgs, (key, value) =>
let stringified = ''
try {
stringified = JSON.stringify(queryArgs, (key, value) =>
isPlainObject(value)
? Object.keys(value)
.sort()
Expand All @@ -27,6 +29,12 @@ export const defaultSerializeQueryArgs: SerializeQueryArgs<any> = ({
}, {})
: value,
)
} catch (e) {
Copy link
Contributor

@rkofman rkofman Mar 26, 2024 •
edited

Choose a reason for hiding this comment

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

(I'm relatively new to open source process && this project, so if I should keep my comments to the issue rather than the PR, please let me know -- or if I'm missing too much context to be useful)

I suspect that for most use cases, silently swallowing an error is worse than throwing it. We probably don't want a silent divergence in behavior based on the input type.

Copy link
Contributor Author

@riqts riqts Mar 26, 2024 •
edited

Choose a reason for hiding this comment

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

This doesn't actually change the majority of the behaviour by just catching this. As seralizeQueryArgs still runs so if the user has provided a way to handle it changing, or if the query args is actually changing it will still behave as normal.

I have a test added to indicate that the base render assumptions don't change with this addition. Is that what you are referring to? or the actual handling. Because I definitely want guidance on the best approach to handling the error rather than my dev mode warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I have a thought! Let me put together a PR.

Copy link
Contributor

@rkofman rkofman Mar 30, 2024 •
edited

Choose a reason for hiding this comment

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

See: #4315

if (
typeof process !== 'undefined' &&
process.env.NODE_ENV === 'development'
) console.warn("Failed to serialize query args for hook", endpointName, e)
}
if (isPlainObject(queryArgs)) {
cache?.set(queryArgs, stringified)
}
Expand Down
98 changes: 75 additions & 23 deletions packages/toolkit/src/query/tests/buildHooks.test.tsx
View file
Open in desktop
Expand Up @@ -74,7 +74,7 @@ const api = createApi({
}
},
endpoints: (build) => ({
getUser: build.query<{ name: string }, number>({
getUser: build.query<{ name: string }, number | bigint>({
query: () => ({
body: { name: 'Timmy' },
}),
Expand All @@ -100,7 +100,7 @@ const api = createApi({
getError: build.query({
query: () => '/error',
}),
listItems: build.query<Item[], { pageNumber: number }>({
listItems: build.query<Item[], { pageNumber: number | bigint }>({
serializeQueryArgs: ({ endpointName }) => {
return endpointName
},
Expand Down Expand Up @@ -627,33 +627,85 @@ describe('hooks tests', () => {
)
})

test(`useQuery refetches when query args object changes even if serialized args don't change`, async () => {
function ItemList() {
const [pageNumber, setPageNumber] = useState(0)
const { data = [] } = api.useListItemsQuery({ pageNumber })
describe("serializeQueryArgs handling", () => {
beforeEach(() => {
nextItemId = 0
})

const renderedItems = data.map((item) => (
<li key={item.id}>ID: {item.id}</li>
))
return (
<div>
<button onClick={() => setPageNumber(pageNumber + 1)}>
Next Page
</button>
<ul>{renderedItems}</ul>
</div>
)
}
test(`useQuery refetches when query args object changes even if serialized args don't change`, async () => {
function ItemList() {
const [pageNumber, setPageNumber] = useState(0)
const { data = [] } = api.useListItemsQuery({ pageNumber })

const renderedItems = data.map((item) => (
<li key={item.id}>ID: {item.id}</li>
))
return (
<div>
<button onClick={() => setPageNumber(pageNumber + 1)}>
Next Page
</button>
<ul>{renderedItems}</ul>
</div>
)
}

render(<ItemList />, { wrapper: storeRef.wrapper })
render(<ItemList />, { wrapper: storeRef.wrapper })

await screen.findByText('ID: 0')
await screen.findByText('ID: 0')

await act(async () => {
screen.getByText('Next Page').click()
await act(async () => {
screen.getByText('Next Page').click()
})

await screen.findByText('ID: 3')
})

await screen.findByText('ID: 3')
// See https://github.com/reduxjs/redux-toolkit/issues/4279 - current limitation is that the hook will not refetch
// when the user defined serializeQueryArgs remains consistent and the defaultSerializeQueryArgs throws an error
test('useQuery gracefully handles non-serializable queryArgs but does not trigger a refetch', async () => {
function ItemList() {
const [pageNumber, setPageNumber] = useState(0)
const { data = [] } = api.useListItemsQuery({ pageNumber: BigInt(pageNumber) })

const renderedItems = data.map((item) => (
<li key={item.id}>ID: {item.id}</li>
))
return (
<div>
<button onClick={() => setPageNumber(pageNumber + 1)}>
Next Page
</button>
<ul>{renderedItems}</ul>
</div>
)
}

render(<ItemList />, { wrapper: storeRef.wrapper })

await screen.findByText('ID: 0')
})

test('Non-serializable values do not impact base render assumptions', async () => {
function User() {
const { isFetching } = api.endpoints.getUser.useQuery(BigInt(1))
getRenderCount = useRenderCounter()

return (
<div>
<div data-testid="isFetching">{String(isFetching)}</div>
</div>
)
}

render(<User />, { wrapper: storeRef.wrapper })
expect(getRenderCount()).toBe(2)

await waitFor(() =>
expect(screen.getByTestId('isFetching').textContent).toBe('false'),
)
expect(getRenderCount()).toBe(3)
})
})

describe('api.util.resetApiState resets hook', () => {
Expand Down