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.

Sign up for GitHub

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

Jump to bottom

[RED-10] prefetch creates a suscription that can not be unsubscribed #1283

Open
phryneas opened this issue Jul 11, 2021 · 12 comments · May be fixed by #2245
Open

[RED-10] prefetch creates a suscription that can not be unsubscribed #1283

phryneas opened this issue Jul 11, 2021 · 12 comments · May be fixed by #2245
Labels
bug Something isn't working rtk-query
Milestone
Post 2.0

Comments

@phryneas
Copy link
Member

phryneas commented Jul 11, 2021 •
edited by markerikson
Loading

Currently, prefetch creates a subscription that cannot be unsubscribed.

Ideally, prefetch should either

  • subscribe and directly unsubscribe, to just keep the cache data around until cache collection
  • subscribe and return a means to unsubscribe

It could also just take an option to switch between those two behaviours.

RED-10
@phryneas phryneas added the bug Something isn't working label Jul 11, 2021
@bever1337
Copy link
Contributor

bever1337 commented Jan 18, 2022 •
edited
Loading

Are there ideas for how usePrefetch could return unsubscribe without a breaking change? Technically you can assign a function as a property of another function, but I don't think that's a good solution for a public API.

Running unsubscribe on the thunk's finally inside prefetch seems pretty simple by comparison.

Edit: I've reviewed the source more and now understand this isn't a relevant question. Nothing is returned now, so the change would not be breaking

bever1337 pushed a commit to bever1337/redux-toolkit that referenced this issue Jan 18, 2022
@bbboydston
feat(query): Adds subscribe option to prefetch, automatically unsubsc…
a88be56 
…ribes if not present [reduxjs#1283]
@bever1337 bever1337 mentioned this issue Jan 18, 2022
Draft
@bever1337
Copy link
Contributor

bever1337 commented Jan 18, 2022 •
edited
Loading

In PR #1938 I toyed around with prefetch resolving void OR the query result, exposing unwrap, unsubscribe, etc. That was pretty cool I thought. 5 seconds later, I realized this is a convoluted path to get the same behavior as manually initiating a query. I don't know why anyone would want the current behavior, but this does represent a change.

@bever1337
Copy link
Contributor

bever1337 commented Jan 18, 2022 •
edited
Loading

The next approach will be to provide a config option like cleanup, where false is the default (and current) behavior. Unsubscribing by default is a breaking change, so the last commit needs to be stepped back. There's still no need to expose unsubscribe in my opinion. Long-term, I think the default behavior should be to cleanup and a dangling subscription should be opt-in. I imagine use-cases like an endpoint that provides context for an entire application.

FaberVitale added a commit to FaberVitale/redux-toolkit that referenced this issue Apr 24, 2022
@FaberVitale
feat(rtk-query): unsubscribe prefetched queries after configurable ti…
f994418 
…mer reduxjs#1283

Closes reduxjs#1283

Automatically removes prefetch subscriptions after configurable amount of time.

Description:

Prefetch subscription are now removed after  `prefetchOptions.keepSubscriptionFor` if provided
or api.config.keepPrefetchSubscriptionsFor otherwise.

Api changes:

- adds `keepSubscriptionFor` to prefetchOptions
- adds `keepPrefetchSubscriptionsFor` to api.config (default 10s)

Internal changes:

- prefetch queries now have the same requestId and the same subscription key
@FaberVitale FaberVitale linked a pull request Apr 24, 2022 that will close this issue
Open
5 tasks
FaberVitale added a commit to FaberVitale/redux-toolkit that referenced this issue May 8, 2022
@FaberVitale
feat(rtk-query): unsubscribe prefetched queries after configurable ti…
4594d4d 
…mer reduxjs#1283

Closes reduxjs#1283

Automatically removes prefetch subscriptions after configurable amount of time.

Description:

Prefetch subscription are now removed after  `prefetchOptions.keepSubscriptionFor` if provided
or api.config.keepPrefetchSubscriptionsFor otherwise.

Api changes:

- adds `keepSubscriptionFor` to prefetchOptions
- adds `keepPrefetchSubscriptionsFor` to api.config (default 10s)

Internal changes:

- prefetch queries now have the same requestId and the same subscription key
FaberVitale added a commit to FaberVitale/redux-toolkit that referenced this issue May 8, 2022
@FaberVitale
feat(rtk-query): prefetch thunk return type reduxjs#1283
36f52db 
Description:

prefetch thunk now returns an object with shape:

```ts
export interface PrefetchActionCreatorResult {
  /**
   * Returns a promise that **always** resolves to `undefined`
   * when there are no pending requests initiated by input thunk.
   */
  unwrap(): Promise<void>,
  /**
   * Cancels pending requests.
   */
  abort(): void
}
```
FaberVitale added a commit to FaberVitale/redux-toolkit that referenced this issue Jun 11, 2022
@FaberVitale
feat(rtk-query): unsubscribe prefetched queries after configurable ti…
32a542c 
…mer reduxjs#1283

Closes reduxjs#1283

Automatically removes prefetch subscriptions after configurable amount of time.

Description:

Prefetch subscription are now removed after  `prefetchOptions.keepSubscriptionFor` if provided
or api.config.keepPrefetchSubscriptionsFor otherwise.

Api changes:

- adds `keepSubscriptionFor` to prefetchOptions
- adds `keepPrefetchSubscriptionsFor` to api.config (default 10s)

Internal changes:

- prefetch queries now have the same requestId and the same subscription key
FaberVitale added a commit to FaberVitale/redux-toolkit that referenced this issue Jun 11, 2022
@FaberVitale
feat(rtk-query): prefetch thunk return type reduxjs#1283
1f17ca7 
Description:

prefetch thunk now returns an object with shape:

```ts
export interface PrefetchActionCreatorResult {
  /**
   * Returns a promise that **always** resolves to `undefined`
   * when there are no pending requests initiated by input thunk.
   */
  unwrap(): Promise<void>,
  /**
   * Cancels pending requests.
   */
  abort(): void
}
```
@vladamx
Copy link

vladamx commented May 24, 2023 •
edited
Loading

@phryneas Bumping this since it can really lead to unpredictable behavior in the app. In general, subscription management is not straightforward.

@markerikson markerikson added this to the 2.0 milestone May 24, 2023
@markerikson
Copy link
Collaborator

@vladamx could you clarify what you mean by "subscription management is not straightforward" ?

@vladamx
Copy link

vladamx commented Jun 5, 2023

@markerikson I mainly meant that hanging or global subscriptions are not easy to find just by statically reading the code. Those subscriptions cause data to never update regardless of keepUnusedFor. Devtools help to spot their count but does not pinpoint where are those subs.

@dorsharonfuse
Copy link

dorsharonfuse commented Jun 27, 2023 •
edited
Loading

@markerikson This literally just introduce a bug in our app where the cache wouldn't clean up because of that dangling subscription.
This needs to be addressed..

However, I think the unsubscription should happen automatically.
Then again, doesn't that mean you could be making the new cache stale right away?

@markerikson
Copy link
Collaborator

markerikson commented Jun 27, 2023 •
edited
Loading

@dorsharonfuse honestly, we're pretty busy with a lot of other things right now. We'll happily accept PRs that fix an issue like this, but this is not a priority for us maintainers to work on atm.

@markerikson markerikson added the linear Created by Linear-GitHub Sync label Sep 22, 2023
@markerikson markerikson changed the title prefetch creates a suscription that can not be unsubscribed [RED-10] prefetch creates a suscription that can not be unsubscribed Sep 22, 2023
@markerikson
Copy link
Collaborator

This is still a thing we'd like to improve, but we're going to focus 2.0 on the existing packaging changes, and defer pretty much all RTKQ changes to a follow-up release (including a likely RTK 3.0 in the semi-near future that would contain any actual breaking changes to RTKQ's API).

Still happy to accept a PR that improves this!

@markerikson markerikson modified the milestones: 2.0, Post 2.0 Oct 1, 2023
@markerikson markerikson removed the linear Created by Linear-GitHub Sync label Feb 7, 2024
@markerikson markerikson closed this as not planned Won't fix, can't repro, duplicate, stale Mar 22, 2024
@markerikson markerikson reopened this Mar 24, 2024
@llianand94
Copy link

we still waiting for this feature! It would be great. Thanks

@fatihgnc
Copy link
Contributor

this is still present, right?

@markerikson
Copy link
Collaborator

We haven't had time yet this year to do any further work on RTKQ, so no changes yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working rtk-query
Projects
None yet
Milestone
Post 2.0
Development

Successfully merging a pull request may close this issue.

feat: add useSuspendAll hook & react/suspense example FaberVitale/redux-toolkit
7 participants
@markerikson @phryneas @vladamx @bever1337 @fatihgnc @dorsharonfuse @llianand94