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

Add createQueryStore #6325

Open
wants to merge 42 commits into
base: develop
Choose a base branch
from
Open

Conversation

christianbaroni
Copy link
Member

@christianbaroni christianbaroni commented Dec 11, 2024

Fixes APP-2154

What changed (plus any additional context for devs)

  • Adds a new Zustand store creator, createQueryStore, which is a version of createRainbowStore with built-in fetching functionality, designed to be a more performant, more composable alternative to useQuery, for most use cases
  • Serves as a replacement for the current useQuery → Zustand sync pattern, which is inefficient and redundant
  • Adds automatic Map and Set persistence support to createRainbowStore
  • Adds a default partializer to createRainbowStore that automatically omits top-level store methods, which previously were being persisted when partialize was not specified

How It Works

  • The query store is internally aware of whether there are mounted components subscribed to its data via selectors — if there are, it handles refetching any time data becomes stale, and if there are not, the store remains or becomes dormant and stops refetching
  • Each query store maintains its own cache (optionally), which means serialization operations are local, limited in scope to each individual store's persisted state — there's no concept of a global cache as exists in RQ, which results in significantly lower overall serialization costs
  • Params are declared once per store, at the store level:
    params: {
      address: ($, store) => $(store).address, // Subscribe to internal query store state
      currency: $ => $(useSettingsStore).currency, // Subscribe to another store's state
      version: 2, // Static param
    }
    • As a result, components re-render only when the data they access changes (not when params change), and param definitions exist in one spot, logically centralizing how, when, and why data is fetched
    • Params can be either:
      • Live subscriptions to internal query store state
      • Live subscriptions to another Zustand store's state
      • Static
    • The dynamic params implementation repurposes ideas from zustand-signal. The $ function transforms a slice of a Zustand store into a live AttachValue. Under the hood, it’s a proxy that subscribes to changes in that slice of the store's state. When the specified state updates, the proxy immediately detects this, forcing the query store to recompute its internal queryKey based on the new params and refetch if needed.
    • See the API Examples below for more details on the dynamic params $ API
  • For a detailed overview of the API, see:

Notes

  • There's a bit of room for further type improvements
    • For instance set and get within the custom state creator currently are not aware of the query store's internal state
    • I may get around to these improvements before this PR is merged but I'd consider it non-blocking, as it doesn't materially affect or limit functionality in the vast majority of cases
  • May want to add a helper setting for a manual mode, where no automatic refetching would occur (perhaps with the exception of the initial fetch), relying only on manually calling fetch for data updates
    • Currently you can achieve something close to this by specifying staleTime: Infinity (first-time fetches will still occur)
    • Update: This is now achievable with disableAutoRefetching

API Examples

The API In Its Simplest Form:

export const useBrowserDappsStore = createQueryStore<Dapp[]>(
  { fetcher: fetchDapps },
);

const MyComponent = () => {
  // Get data for the current params from the built-in query cache
  const top10Dapps = useBrowserDappsStore(state => state.getData()?.slice(0, 10));

  return <FeaturedDapps dapps={top10Dapps} />;
};

Dynamic $ Params:

Narrow state subscriptions linked directly to either internal or external store state.

export const useUserAssetsTestStore = createQueryStore<ParsedAssetsDictByChain, UserAssetsQueryParams, UserAssetsTestStore>(
  {
    fetcher: ({ address, currency }) => simpleUserAssetsQuery({ address, currency }),
    params: {
      address: ($, store) => $(store).address, // Subscribe to internal query store state
      currency: $ => $(useSettingsStore).userPrefs.currency, // Subscribe to another store's state
      version: 2, // Static param
    },
    staleTime: time.minutes(1),
  },

  // Optional custom store state
  set => ({
    address: testAddresses[0],
    setAddress: (address: Address) => set({ address }),
  }),

  // Optional RainbowPersistConfig (same API as createRainbowStore)
  { storageKey: 'queryStoreTest' }
);

No Params & Overriding Built-in Data Caching via setData:

export const useBrowserDappsStore = createQueryStore<Dapp[], never, DappsState>(
  {
    fetcher: fetchDapps,
    setData: ({ data, set }) => set({ dapps: data }),
    cacheTime: time.days(2),
    staleTime: time.minutes(20),
  },

  (_, get) => ({
    dapps: [],
    findDappByHostname: (hostname: string) => get().dapps.find(dapp => dapp.urlDisplay === hostname),
  }),

  { storageKey: 'browserDapps' }
);

Screen recordings / screenshots

Two copies of the same query store implementation, each with their own state and a unique address param, both leveraging persistence and dynamic $ params (you wouldn't want two copies of the same store in practice, that's purely for testing purposes):

ScreenRecording_12-23-2024.18-02-12_1.MP4

What to test

  • There's an example query store and component here, which can be rendered anywhere in the app for testing: QueryStoreTest.tsx
    • Planning to comment out this file before merging to avoid any effect on prod — I think it's useful to keep around for testing and further store improvements

Copy link

linear bot commented Dec 11, 2024

@christianbaroni christianbaroni changed the title Add createRemoteRainbowStore Add createRainbowQueryStore Dec 16, 2024
@christianbaroni christianbaroni changed the title Add createRainbowQueryStore Add createQueryStore Dec 17, 2024
@brunobar79
Copy link
Member

Launch in simulator or device for bcade4c

…uracy, make fetch return data, expose params to transform, misc. optimizations
@brunobar79
Copy link
Member

Launch in simulator or device for 1a53f12

Copy link

socket-security bot commented Jan 1, 2025

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/[email protected] None 0 16.2 kB formidablelabs

🚮 Removed packages: npm/[email protected]

View full report↗︎

@brunobar79
Copy link
Member

Launch in simulator or device for c2f7013

@brunobar79
Copy link
Member

Launch in simulator or device for 8dd74f9

Copy link
Contributor

@maxbbb maxbbb left a comment

Choose a reason for hiding this comment

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

This is a really nice.

Only concerns are around the functionality of cacheTime and staleTime, but it's kind of subjective. In general, I think how those behave should stay closer to how they behave in react query to make transitioning from it simpler. A short staleTime can be useful when you know a component might get mounted / unmounted in a short enough timespan that you wouldn't want to trigger another fetch, but in those scenarios you don't want to also have it refetching at that same time interval.

For cacheTime I would expect that after the cacheTime has elapsed that the cached data would no longer be returned. Right now the cache isn't pruned until after the fetch, which means the component would get expired data until the fetch was complete. There should be some way to evict the cache after it is expired to prevent it from being returned when the initial fetch is happening.

For staleTime, I do think it should be split into a separate staleTime and refetchInterval to make it simpler to opt out of refetches while still maintaining control of when the data should be considered stale.

Comment on lines +311 to +330
const [persist, discard] = [true, false];

const SHOULD_PERSIST_INTERNAL_STATE_MAP: Record<string, boolean> = {
/* Internal state to persist if the store is persisted */
enabled: persist,
error: persist,
lastFetchedAt: persist,
queryCache: persist,
queryKey: persist,
status: persist,

/* Internal state and methods to discard */
fetch: discard,
getData: discard,
getStatus: discard,
isDataExpired: discard,
isStale: discard,
reset: discard,
subscriptionCount: discard,
} satisfies Record<InternalStateKeys, boolean>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think assigning variables here adds any clarity over just assigning true or false directly

const SHOULD_PERSIST_INTERNAL_STATE_MAP: Record<string, boolean> = {
  /* Internal state to persist if the store is persisted */
  enabled: true,
  error: true,
  lastFetchedAt: true,
  queryCache: true,
  queryKey: true,
  status: true,

  /* Internal state and methods to discard */
  fetch: false,
  getData: false,
  getStatus: false,
  isDataExpired: false,
  isStale: false,
  reset: false,
  subscriptionCount: false,
} satisfies Record<InternalStateKeys, boolean>;

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

src/state/internal/createQueryStore.ts Show resolved Hide resolved
src/state/internal/createQueryStore.ts Outdated Show resolved Hide resolved
…dant work when multiple subscribed components are mounted, reduce subscription handler redundancy, expose more tools to onFetched, clean up lastFetchedAt logic, other small improvements
@brunobar79
Copy link
Member

Launch in simulator or device for 867699c

@brunobar79
Copy link
Member

Launch in simulator or device for f4948ba

@brunobar79
Copy link
Member

Launch in simulator or device for 77a2e67

@brunobar79
Copy link
Member

Launch in simulator or device for b46deec

@brunobar79
Copy link
Member

Launch in simulator or device for 910f4f9

@brunobar79
Copy link
Member

Launch in simulator or device for f4a54b1

@brunobar79
Copy link
Member

Launch in simulator or device for f3ccce2

Copy link
Contributor

@walmat walmat left a comment

Choose a reason for hiding this comment

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

Code looks good. I need to test closer for potential memory leaks with timeouts etc. but first glance looks fine.

Comment on lines +311 to +330
const [persist, discard] = [true, false];

const SHOULD_PERSIST_INTERNAL_STATE_MAP: Record<string, boolean> = {
/* Internal state to persist if the store is persisted */
enabled: persist,
error: persist,
lastFetchedAt: persist,
queryCache: persist,
queryKey: persist,
status: persist,

/* Internal state and methods to discard */
fetch: discard,
getData: discard,
getStatus: discard,
isDataExpired: discard,
isStale: discard,
reset: discard,
subscriptionCount: discard,
} satisfies Record<InternalStateKeys, boolean>;
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Comment on lines +434 to +440
if (IS_DEV && !suppressStaleTimeWarning && staleTime < MIN_STALE_TIME) {
console.warn(
`[createQueryStore${persistConfig?.storageKey ? `: ${persistConfig.storageKey}` : ''}] ❌ Stale times under ${
MIN_STALE_TIME / 1000
} seconds are not recommended.`
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should just override the value if it's beneath this value. I don't know if you see a reason for having it exist, but seems to me that it should be disallowed.

Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to create a query that fetches freshest data with staleTime = 0, but that doesn't need to be refetched on an interval by setting disableAutoRefetching

Comment on lines +477 to +478
const abortController = new AbortController();
activeAbortController = abortController;
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to create a local variable here instead of just assigning directly to activeAbortController?

Copy link
Member Author

Choose a reason for hiding this comment

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

Used here to make sure that activeAbortController is only nulled out if it hasn't been reassigned:

} finally {
if (activeAbortController === abortController) {
activeAbortController = null;
}
}

@brunobar79
Copy link
Member

Launch in simulator or device for c21a1f3

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

Successfully merging this pull request may close these issues.

5 participants