-
-
Notifications
You must be signed in to change notification settings - Fork 458
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
(core) - Improve Suspense implementation and fix client-side Suspense #1123
Conversation
🦋 Changeset detectedLatest commit: 973469a The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
const signal = [cache] as [T] & { tag: 1 }; | ||
signal.tag = 1; | ||
sink(signal); | ||
} else { | ||
hasSuspended = true; | ||
sink(0 /* End */); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really isn't great that we're manually sending source events here in case of breaking changes in Wonka (which are in fact coming) but I haven't really found a better implementation for this yet that's as short. The alternative is to use Wonka.make
I suppose but then we need to manually pipe all events through.
Although if anyone would like to try that out; here's the code:
An example of a make-based implementation
/** Convert the Source to a React Suspense source on demand */
function toSuspenseSource<T>(source: Source<T>): Source<T> {
const shared = share(source);
let cache: T | void;
let resolve: (value: T) => void;
return make(observer => {
let hasSuspended = false;
const sub = pipe(
shared,
takeWhile(result => {
// The first result that is received will resolve the suspense
// promise after waiting for a microtick
return !hasSuspended;
}),
subscribe(result => {
if (cache === undefined) Promise.resolve(result).then(resolve);
cache = result;
observer.next(result);
})
);
// If we haven't got a previous result then start suspending
// otherwise issue the last known result immediately
if (cache !== undefined) {
observer.next(cache);
} else {
hasSuspended = true;
observer.complete();
throw new Promise<T>(_resolve => {
resolve = _resolve;
});
}
return sub.unsubscribe;
});
};
Resolve #1115
Summary
This reimplementation of our React Suspense support should improve edge-cases, support the
context.suspense === false
switch consistently, and fix the issue described in #1115 where uncached results would cause an infinite update loop when client-side Suspense was used.The suspense logic has effectively moved to
useQuery
itself and caches result sources temporarily. In its implementation it assumes that all requests eventually are remounted, which is a safe assumption to make since after React Suspense triggers there's no way for a user to abort its eventual remount. Even if the cache leaks a suspense source, this source doesn't stay active forever and the added memory overhead would in theory be minimal.Set of changes
operation.context.suspense
more consistent. It now explicitly determines whether an operation has Suspense-mode enableduseSource
trigger effect touseQuery
anduseSubscription
effectively (which also fixes the case ofuseSubscription
triggering suspense unnecessarily in some casestoSuspenseSource
logic fromClient
and move it touseQuery
with an added hook-wide cachepollInterval
implementation toexecuteRequestOperation
in case we find a way to reduceuseQuery
's bundlesize impact now