[breaking fix] replace `getRunningOperationPromise(s)` with `getRunning(Query|Queries|Mutation|Mutations)Thunk`

[phryneas]

Motivation

As reported in #2477, currently doing await api.util.getRunningOperationsPromise() in a SSR environment will not only await for queries and mutations of the page currently being rendered, but of all other pages being SSRed at the same time. When designing the api, I forgot that multiple SSR renders will likely share the same api instance, but use different stores for that.

Unforunately, with the existing api there is no way fixing this. The only way of scoping these functions to a certain store is to change the way they are called:

-await Promise.all(api.util.getRunningOperationPromises())
+await Promise.all(dispatch(api.util.getRunningOperationPromises()))

We essentially need to do a "breaking fix", as much as it hurts me.

If doing this change directly on the original methods, old code (without dispatch) would keep running without any errors, but get even worse behaviour: all those await statements would immediately continue, resolving to the thunk and not waiting for any running queries and mutations.

As this is too much of a foot gun, this PR needs to introduce even bigger changes.

Changes

• replace getRunningOperationPromise and getRunningOperationsPromise with stubs that immediately throw with a link to the documentation.
• change the type of getRunningOperationPromise and getRunningOperationsPromise to never, but keep them in the types so we can add a link to the documentation there as well (and mark them as @deprecated).
• create four new functions:
  • api.util.getRunningQueryThunk(endpointName, queryArgs)
  • api.util.getRunningMutationThunk(endpointName, fixedCacheKeyOrRequestId)
  • api.util.getRunningQueriesThunk()
  • api.util.getRunningMutationsThunk()

So both functions are split up into two separate functions for queries and mutations, and the word Promise has been replaced by Thunk.

That changes the usage like this:

-await Promise.all(api.util.getRunningOperationPromises())
+await Promise.all([
  ...dispatch(api.util.getRunningQueriesThunk()),
  ...dispatch(api.util.getRunningQueriesThunk()),
])

or more realistically like this

-await Promise.all(api.util.getRunningOperationPromises())
+await Promise.all(dispatch(api.util.getRunningQueriesThunk()))

as it should almost never happen that a mutation is initiated during SSR.

The "breaking fix" compromise

I kept in getRunningOperationPromises for now. The function will throw in development, but keep it's old faulty behaviour in production builds so things will not immediately break beyond the state it was already broken before.

I could not create a version of getRunningOperationPromise that kept the old behaviour, so it throws in both development and production. Luckily, that function was documented as "primarily added to add experimental support for suspense", so the impact of removing it should be quite low.

Todos

• [ ] tests
• [ ] update the example SSR repo at https://github.com/phryneas/ssr-experiments (this has to be coordinated with the release of the next RTK version)
• [ ] maybe provide a codemod (that allows to choose between both refactorings above) - could you take a stab at that @markerikson ?

[codesandbox-ci[bot]]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a73aee9f2c1cb23d00a45f3a9114570718e2aacc:

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration
rsk-github-issues-example	Configuration
@examples-query-react/basic	Configuration
@examples-query-react/advanced	Configuration
@examples-action-listener/counter	Configuration

[netlify[bot]]

✅ Deploy Preview for redux-starter-kit-docs ready!

Name	Link
🔨 Latest commit	e4eb9d0c54203daefa9b4fbbd3856c41e03a81c1
🔍 Latest deploy log	https://app.netlify.com/sites/redux-starter-kit-docs/deploys/634ad1928fb4250008305bd7
😎 Deploy Preview	https://deploy-preview-2481--redux-starter-kit-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[JulienKode]

I'm not as experience as you in open source and in this codebase but from what I saw:

For the fix I've try to take a look at the codebase and maybe value like runningPromises should be create inside a middleware but it seems a big refactoring step to do. As middleware are build with buildMiddleware

I think for a fix it's nice as:

• getRunningOperationPromises become a thunk (which is needed to be scope per store) so if refactoring needs to be made to move runningPromises inside a middleware this can be done without changing the API for the user.

I'll test the fix and let you know, Thank you again for your help

[JulienKode]

Hi @phryneas

I did some test and it clearly seems to fix the issue, Thank you

[markerikson]

@3imed-jaberi I appreciate the enthusiasm, but I would appreciate it if you didn't go through all of our PRs and leave "+1 approve" comments on them :) It adds to our notifications, and that's kind of annoying.

[3imed-jaberi]

from time to time, I make some reviews/PRs/issues ... I apologize for that! I just try to help ... 😅!

It would be better to see labels that tell everyone which stage of the current issue/PR to prevent over actions

[markerikson]

Yeah, I do appreciate the spirit :) But, tbh generally other folks don't have the context to properly review these PRs. Like, in this case, the changes in this PR aren't necessarily done, and there's a lot of complex nuance as to whether we even want to merge this PR in its current form. So, a "+1 approved" from someone else isn't really helpful.

What we could use help with is triaging other actual issues.

As far as labels and stuff go: yeah, those can be useful, but those also require a lot of time and effort to keep updated (and like I said, we don't generally expect folks who aren't one of the maintainers to be worrying about those anyway).

[phryneas]

Okay, updated code & docs as well as the PR description itself - @markerikson do we want to go forward with it like I suggest above?

[markerikson]

Looking at https://sourcegraph.com/search?q=context:global+getRunningOperationPromise+-repo:reduxjs+-repo:phryneas+-file:node_modules+-language:markdown&patternType=standard , I can only see 14 actual uses of getRunningOperationPromise[s] in the wild. That's small enough that I'm okay with calling this a minor version change even though it's technically breaking. Also that it's conceptually a "bug fix".

We can file issues on those repos separately, or even do the PRs ourselves.

Also given that, I don't think it's worth putting the effort into a codemod.

[nasir733]

can somebody help me by providing a complete example snippet for getserversideprops . I am confused about where is the dispatch coming from in the code snippet example in the docs await Promise.all(dispatch(api.util.getRunningQueriesThunk())) @phryneas
regards nasir

[phryneas]

@nasir733 Well... dispatch is not defined We assume that you have a function called dispatch, but apparently you were calling store.dispatch - do you have to use that.

[browynlouis]

I'm trying to run rtkQuery during SSR, all goes well until it gets to dispatching the getRunningQueriesThunk and I get the following function TypeError: _features_api_subApis_propertyApi__WEBPACK_IMPORTED_MODULE_4__.propertyApi.util.getRunningQueriesThunk is not a function

export const getServerSideProps = reduxWrapper.getServerSideProps(
  (store) => async (context) => {
    const propertyId = context.params?.propertyId;

    store.dispatch(getProperty.initiate(propertyId));

    const response = await Promise.all(
      store.dispatch(propertyApi.util.getRunningQueriesThunk())
    );
    console.log(response);

    return {
      props: {},
    };
  }
);

I have tried looking for solution online, but I found none, When I check the methods/functions of the api.util, I see the old getRunningOperationPromises and cannot find getRunningQueriesThunk Please how can I solve this?

[markerikson]

@browyn what version of RTK do you have installed?

[browynlouis]

It's version 1.8.6 The code base is legacy 🥲

[browynlouis]

Perhaps, I should take my time to update the entire code base. Been worried about that, so I don't get to break anything.

I just tried created a new project just to test out the getRunningQueriesThunk and it works

[markerikson]

@browyn yeah, we released the fix in 1.9.0 :) You just need to upgrade to 1.9.x.

[browynlouis]

Thanks @markerikson

Problem solved!

[shinhyogeun]

hi. @markerikson i'm in 1.9.5 version

and i make custom hook for Suspense

but i got error in this code.

...

const info = useEndpointQuery(params, { ...(options || {}) });

if (info.isError) {
    throw info.error;
}

  if (info.isFetching) {
    const promise = dispatch(api.util.getRunningQueryThunk(endpointName, params));

    throw promise
  }

because const promise is undefined in first time.

if i console.log promise value .

but i have to throw promise right from the start for suspense.

Can you give me some advice? 🥲

[markerikson]

@shinhyogeun that's expected, yes. The query doesn't actually start until a useEffect runs after the render.

We don't have any official support for Suspense yet, although we're kicking around some ideas.

[shinhyogeun]

thank you 🙏 @markerikson

i found if i write code like this. i can get promise right from the start.

if (info.isError) {
    throw info.error;
  }

  if (Loading) {
    dispatch(api.endpoints[`${endpointName}`].initiate(params));

    const promise = dispatch(api.util.getRunningQueryThunk(endpointName, params));

     throw promise;
  }

why is this possible?

and i also wonder if getRunningQueryThunk can give promise every time it polling.

i also check your code in https://github.com/replayio/devtools/pull/7205/files#diff-ffc9c20da71515be364ceb20ce82f542e4b96ae7fec51a2bb769d3dcc338ba84

[phryneas]

why is this possible?

Because initiate starts the request by hand. But be careful, that creates a subscription and will never be collected from cache that way.