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

Support for explicit resource management "using statements" on RTK Query subscriptions #3708

Closed
wants to merge 1 commit into from

Conversation

jared-ca
Copy link
Contributor

Issue: #3707

This PR proposes the addition of a [Symbol.dispose]: () => void to be added to the object returned from dispatch(endpoint.initiate()) to enable explicit resource management with using statements.

I've put effort into ensuring that none of the exported types reference Symbol.dispose when it's not available so consumers with older versions of Typescript won't have any type errors.

There are some explicit any casts within disposableIfAvailable.ts that allows this change to be compiled without needing to update the whole library to use Typescritp 5.2+ We're currently on 4.2.4. These any types are only internal to the module.

Testing

I've manually tested this change using the example code snippet in #3707

Add a dispose method to the initiate action to allow subscriptions to be managed with the using keyword.
@codesandbox
Copy link

codesandbox bot commented Sep 10, 2023

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@codesandbox-ci
Copy link

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 a41a4d5:

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

@phryneas
Copy link
Member

phryneas commented Sep 10, 2023

Thanks for the PR!

While I generally love using (proof here 😅 ), I'm not sure if it's actually useful in the case of initiate, or if this case would just hide/work around another bug.

If you really want to expose of the store value within a scope, chances are you don't really care about a store subscription in the first place, but more about the promise return value.

Right now we have the bug that if you call initiate with { subscribe: false }, and no cache entry exists yet, the Promise returns a bogus value.
That would be solved with #3709 - so I'd rather have people using initiate(x, {subscribe: false}) instead of adding using here to create something "long-lived" where we don't really care about it being "long-lived" at all.

@jared-ca
Copy link
Contributor Author

jared-ca commented Sep 10, 2023

Cheers @phryneas,

I think you're probably right that most people only care about the promise!

In my case I actually care about the subscription and want to ensure the response stays in the cache for the lifetime of the scope because I have a bunch of computationally expensive business logic in memoized selectors that I want to make use of.

So in practice I'm writing listener middleware that looks something like this:

const listenerMiddleware = createListenerMiddleware()
listenerMiddleware.startListening({
  actionCreator: someAction,
  effect: async (action, api) => {
    using request1 = api.dispatch(endpoints.request1.initiate());
    using request2 = api.dispatch(api.endpoints.request2.initiate());
    using request3 = api.dispatch(api.endpoints.request3.initiate());
    using request4 = api.dispatch(api.endpoints.request4.initiate());

    await (Promise.all([request1, request2, request3, request4]));

    const shouldPerformAction = selectExpensiveUserIsCool(api.getState());
    if (shouldPerformAction) {
      await doSomethingElse();
    }
  }
})

Maybe I'm doing things a bit oddly but I love the pattern of having a flat redux store and then doing the cross store complex data wrangling in selectors, leveraging re-select memozing them all and re-evaluating them only when the dependent state changes.

Selectors are the hammer I like and everything looks like a nail perhaps but I end up with quite a large tree of memoized re-select selectors 😂

From the React side it's great, just use() all the dependent apis and then the selector but using the same selectors outside of React, in this case in a middleware is a bit more awkward.

@markerikson markerikson added this to the Post 2.0 milestone Oct 14, 2023
@EskiMojo14
Copy link
Collaborator

closing as the linked issue has been closed - thanks for the PR regardless!

@EskiMojo14 EskiMojo14 closed this Feb 12, 2024
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.

4 participants