Proof of concept: create an RTKQ-based React Suspense cache for the sources list #7205

markerikson · 2022-06-17T21:57:35Z

This PR:

Adds a HIGHLY EXPERIMENTAL POC (as in "look I tried to understand Brian's code and hacked on this for a couple hours and it amazingly seems to actually do something useful 🤷‍♂️ ") React Suspense cache built on top of RTK Query's getRunningOperationPromise API
Updates the <SourcesList> component to read from that cache, and wraps it in a <Suspense> + loader (with an artificial delay added to the RTKQ API endpoint just to see that it's showing the fallback)
Turns on strict: true in the tsconfig.json for both prototype folders, because APPARENTLY WE DIDN'T HAVE TS STRICT MODE ON AND THIS WAS BREAKING THINGS ARGHGHHHHGGH!

vercel · 2022-06-17T21:57:37Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
devtools	✅ Ready (Inspect)	Visit Preview	Jul 1, 2022 at 8:51PM (UTC)

qa-wolf · 2022-06-17T22:12:12Z

✅ QA Wolf - Deploy PR

Hello here: 7 tests ran, see details here.

✅ 7 passed

✅ Share replay via invite link: only people with access
✅ View and click events
✅ View and search library
✅ View footer links: about
✅ Share replay via invite link: members of Test team
✅ User role unable to: access billing, view api keys, delete team, edit team name
✅ Share replay via invite link: members of QAWolf :: Facebook

bvaughn · 2022-06-21T19:13:19Z

About to review this change set but this change would be nice to extract into its own PR (just in case it causes problems or we need to revert something for some reason).

Turns on strict: true in the tsconfig.json for both prototype folders, because APPARENTLY WE DIDN'T HAVE TS STRICT MODE ON AND THIS WAS BREAKING THINGS ARGHGHHHHGGH!

markerikson · 2022-06-21T19:18:24Z

@bvaughn sure, filed #7236 with the tsconfig change and I can rebase this after that's in

bvaughn · 2022-06-21T19:15:01Z

packages/markerikson-stack-client-prototype/src/app/api.ts

 export const api = createApi({
  baseQuery: fakeBaseQuery(),
  endpoints: build => ({
    getSources: build.query<SourceGroups, void>({
      queryFn: async () => {
        const sources: newSource[] = [];

+        // TODO Artificial delay just to see the "Loading..." for Suspense
+        await sleep(3000);


Might want to remove this before actually merging the changeset?

bvaughn · 2022-06-21T19:15:32Z

packages/markerikson-stack-client-prototype/src/features/sources/SourcesList.tsx

+  // const { data } = useGetSourcesQuery();
+  console.log("SourcesList trying to read data via Suspense...");


nit: Remove this before merging?

bvaughn · 2022-06-21T19:19:04Z

packages/markerikson-stack-client-prototype/src/pages/index.tsx

+import { store } from "../app/store";
+import { setStore } from "../features/sources/sourcesCache";
+
+setStore(store);


Would be nice to avoid relying on this type of module side effect. (Current code base has a lot of those, and it complicates things like the order packages can be imported in etc.)

Maybe the SourcesList component could grab the store from context and pass it to getSourceGroups() instead?

Potentially, yeah, although this is technically a pattern we do recommend for cases where non-React-related modules need access to the store:

https://redux.js.org/faq/code-structure#how-can-i-use-the-redux-store-in-non-component-files

In theory, having it get injected from a root file should mean no import order issues.

I think it's good to avoid globals as much as possible.

Even injecting it from the (application) "root" could lead to awkward cases when writing tests for a component, for example. If we don't require/render the root, but the component uses the hook– then it's going to run into an undefined value error.

bvaughn · 2022-06-21T19:22:43Z

packages/markerikson-stack-client-prototype/src/features/sources/sourcesCache.ts

+  // Create or read the cache for this section of the UI (????)
+  const sourceGroupsRecord = getCacheForType(createSourceGroupsRecord);


Since there's only one set of sources– one thing we need to cache per recording– and only one recording per app session– we don't really need to use getCacheForType. We could literally just use a module-level variable to track our state.

React's (current, experimental) getCacheForType API provides some additional functionality that we don't need in this case:

Ability to invalidate the cache (when data changes) as part of a low-priority update – without blocking high-priority updates that might come up.

Ability to mask a higher level cache with a lower level one. (For example, a higher level cache may be a friends list with only a few user attributes, and a lower level cache might be a user's profile details.)

In our case (1) sources are never invalidated and (2) the recording and its sources are global for an entire session.

In that sense, this cache is more like the React DevTools "hook names" cache:
https://github.com/facebook/react/blob/cf665c4b73a28b034c8173f4d929205fb12d2d2e/packages/react-devtools-shared/src/hookNamesCache.js#L64-L68

This is part of where I really don't know what I'm doing with Suspense Caches :) I was attempting to mimic what I saw over in your prototype folder.

Yeah totally makes sense. Was just sharing some thoughts as I read this but maybe it would be better for us to hop on a VC and talk about it?

bvaughn · 2022-06-21T19:24:16Z

packages/markerikson-stack-client-prototype/src/features/sources/sourcesCache.ts

+  // isFetching = true any time a request is in flight
+  const isFetching = sourcesEntry!.isLoading;
+  // isLoading = true only when loading while no data is present yet (initial load with no data in the cache)
+  const isLoading = !hasData && isFetching;


It's not clear to me why sourcesEntry!.isLoading isn't sufficient for this. RTK should reset that field to false once it has data, no?

RTKQ differentiates between "loading", which is only true during the initial setup, and "fetching", which can go between false/true many times if you change the parameters being passed into a given query hook:

In this case, we're doing the work outside of any hook, so I actually copy-pasted this logic from inside of RTKQ's hooks code. It may not be totally necessary here, I'd have to think about it a bit more.

bvaughn · 2022-06-21T19:24:45Z

packages/markerikson-stack-client-prototype/src/features/sources/sourcesCache.ts

+  // isLoading = true only when loading while no data is present yet (initial load with no data in the cache)
+  const isLoading = !hasData && isFetching;
+  // isSuccess = true when data is present
+  const isSuccess = sourcesEntry!.isSuccess || (isFetching && hasData);


Wait, isFetching and hasData coexist? I'm confused. Doesn't data normally come after fetching completes?

See above comment - for the hooks the parameter can change, and that's where I pulled this code from.

bvaughn · 2022-06-21T19:25:25Z

packages/markerikson-stack-client-prototype/src/features/sources/sourcesCache.ts

+    const promise = api.util.getRunningOperationPromise("getSources", undefined)!;
+    promise.then(result => {


I think if the component were to re-request this data before the promise finishes, you'd end up adding multiple .then() callbacks, wouldn't you?

Hmm. Can that happen? Because the component is suspended and we're waiting for this exact promise to resolve before it will render again.

Yes. React might try re-rendering in some scenarios. This is why it's important that the thing that's thrown is stable, for example. (That would also be true of any event handlers you added to that thing.)

Gotcha. On the other hand, in this particular scenario, is that actually a problem?

The .then is solely for the purpose of mutating the record with the received value, so if it runs more than once it's a no-op. Also, we're throwing the original promise, not the .then promise.

(I was also having a bit of trouble trying to translate the sync "wakeable" logic in your example over to an async promise, which is how I ended up with this setup.)

That's fair.

Unless I'm misunderstanding something, I think this cache could be simplified to the following:

import { unstable_getCacheForType as getCacheForType } from "react"; import { api, SourceGroups } from "../../app/api"; import type { AppStore } from "../../app/store"; let hasPromiseListenerBeenAttached = false; let sourceGroups: SourceGroups | null; const selectSourceGroupsEntry = api.endpoints.getSources.select(); export function getSourceGroups(store: AppStore): SourceGroups { if (sourceGroups !== null) { return sourceGroups; } let sourcesEntry: ReturnType<typeof selectSourceGroupsEntry> | undefined = selectSourceGroupsEntry(store.getState()); if (!sourcesEntry || sourcesEntry.isUninitialized) { store.dispatch(api.endpoints.getSources.initiate()); sourcesEntry = selectSourceGroupsEntry(store.getState()); } const promise = api.util.getRunningOperationPromise("getSources", undefined)!; if (!hasPromiseListenerBeenAttached) { hasPromiseListenerBeenAttached = true; promise.then(result => { sourceGroups = result.data as SourceGroups; }); } throw promise; }

markerikson · 2022-07-01T20:50:09Z

Some updates:

Rebased this on main to pick up all the Yarn changes
Reworked the API + client setup so that the client gets injected into the API file by <Initializer>
Removed the setStore() from the cache file and am now passing it in to getSourceGroups(store)
Replaced my original more complicated implementation with Brian's simpler implementation, since the sources list is a one-time fetch

markerikson requested review from bvaughn, jasonLaster, jcmorrow and jaril June 17, 2022 21:57

replay-soc2 bot approved these changes Jun 17, 2022

View reviewed changes

markerikson force-pushed the feature/FE-259-rtkq-suspense branch from 1e93e30 to cf0e483 Compare June 17, 2022 21:59

vercel bot had a problem deploying to Preview June 17, 2022 22:01 Failure

markerikson force-pushed the feature/FE-259-rtkq-suspense branch from cf0e483 to 0005627 Compare June 17, 2022 22:06

vercel bot deployed to Preview June 17, 2022 22:12 View deployment

markerikson mentioned this pull request Jun 21, 2022

Turn on strict null checks in both prototype folders #7236

Merged

bvaughn reviewed Jun 21, 2022

View reviewed changes

markerikson mentioned this pull request Jun 22, 2022

Support for Suspense in RTK Query reduxjs/redux-toolkit#1574

Open

markerikson added 5 commits July 1, 2022 16:03

Copy "next" React typedefs

901aba1

Remove sessionId from API args and inject at startup

d8dee44

Use an experimental RTKQ-based Suspense Cache for the sources list

766763d

Simplify Suspense cache implementation per Brian's suggestions

c618d5f

Show source entry kinds in list

9e4dfb6

markerikson force-pushed the feature/FE-259-rtkq-suspense branch from 0005627 to 9e4dfb6 Compare July 1, 2022 20:45

vercel bot deployed to Preview July 1, 2022 20:51 View deployment

markerikson merged commit 3c483f6 into main Jul 1, 2022

markerikson deleted the feature/FE-259-rtkq-suspense branch July 1, 2022 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof of concept: create an RTKQ-based React Suspense cache for the sources list #7205

Proof of concept: create an RTKQ-based React Suspense cache for the sources list #7205

markerikson commented Jun 17, 2022 •

edited

Loading

vercel bot commented Jun 17, 2022 •

edited

Loading

qa-wolf bot commented Jun 17, 2022 •

edited

Loading

bvaughn commented Jun 21, 2022 •

edited

Loading

markerikson commented Jun 21, 2022

bvaughn Jun 21, 2022

bvaughn Jun 21, 2022

bvaughn Jun 21, 2022

markerikson Jun 21, 2022

bvaughn Jun 21, 2022

bvaughn Jun 21, 2022

bvaughn Jun 21, 2022

bvaughn Jun 21, 2022

markerikson Jun 21, 2022

bvaughn Jun 21, 2022

bvaughn Jun 21, 2022

markerikson Jun 21, 2022

bvaughn Jun 21, 2022

markerikson Jun 21, 2022

bvaughn Jun 21, 2022

markerikson Jun 21, 2022

bvaughn Jun 21, 2022

markerikson Jun 21, 2022

bvaughn Jun 21, 2022

markerikson commented Jul 1, 2022

		// const { data } = useGetSourcesQuery();
		console.log("SourcesList trying to read data via Suspense...");

		// Create or read the cache for this section of the UI (????)
		const sourceGroupsRecord = getCacheForType(createSourceGroupsRecord);

		const promise = api.util.getRunningOperationPromise("getSources", undefined)!;
		promise.then(result => {

Proof of concept: create an RTKQ-based React Suspense cache for the sources list #7205

Proof of concept: create an RTKQ-based React Suspense cache for the sources list #7205

Conversation

markerikson commented Jun 17, 2022 • edited Loading

vercel bot commented Jun 17, 2022 • edited Loading

qa-wolf bot commented Jun 17, 2022 • edited Loading

✅ QA Wolf - Deploy PR

bvaughn commented Jun 21, 2022 • edited Loading

markerikson commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markerikson commented Jul 1, 2022

markerikson commented Jun 17, 2022 •

edited

Loading

vercel bot commented Jun 17, 2022 •

edited

Loading

qa-wolf bot commented Jun 17, 2022 •

edited

Loading

bvaughn commented Jun 21, 2022 •

edited

Loading