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

refactor(react-api-client): sanitize host object undefined values in all react-api-client hooks #15189

Draft
wants to merge 5 commits into
base: edge
Choose a base branch
from

Conversation

ncdiehl11
Copy link
Collaborator

@ncdiehl11 ncdiehl11 commented May 15, 2024

Overview

React query does not properly handle objects passed in useQuery's queryKey array argument that contain keys with undefined values. Here, I introduce a new utility in React API client that replaces undefined values of an object with null values. This allows for proper caching of objects in queryKey arrays.

reference

Test Plan

  • smoke test throughout app
  • inspect number of requests over time in the app with an without sanitized host included in queryKey

Changelog

  • create utility for converting undefined object values to null
  • implement in every react api client hook using a host object in queryKey

Review requests

Risk assessment

medium, on the basis of touching nearly the entirety of the react api client

ncdiehl11 added 4 commits May 15, 2024 14:28
…all react-api-client hooks

React query does not properly handle objects passed in useQuery's queryKey array argument that
contain keys with undefined values. Here, I introduce a new utility in React API client that
replaces undefined values of an object with null values. This allows for proper caching of objects
in queryKey arrays.
@ncdiehl11 ncdiehl11 self-assigned this May 15, 2024
*
* @returns {Object | null} Returns value-updated object
*/
export function getSanitizedQueryKeyObject(obj: Object | null): Object | null {
Copy link
Member

Choose a reason for hiding this comment

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

If this is specific to sanitizing things, then let’s not have it be (Object | null): Object, let’s have it be either (T | null): T or have an explicit instance that’s ergonomic for the most common use case of host config, which is (Host): HostConfig. Doing that last one (Host might not be the right type name, I mean whatever useHost returns) means we can drop all those as HostConfig clauses at every point of use, which means that if you forget to use this then tsc will yell at you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants