-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Docs for Promise subclasses #8125
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
base: main
Are you sure you want to change the base?
Docs for Promise subclasses #8125
Conversation
e29f977 to
40ace05
Compare
Size changes📦 Next.js Bundle Analysis for react-devThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
src/content/reference/react/use.md
Outdated
| ### Avoiding fallbacks by passing Promise subclasses {/*avoiding-fallbacks-by-passing-promise-subclasses*/} | ||
| React will read the `status` field on Promise subclasses to synchronously read the value without waiting for a microtask. If the Promise is already settled (resolved or rejected), React can read the value immediately without suspending and showing a fallback if the update was not part of a Transition (e.g. [`ReactDOM.flushSync()`](/reference/react-dom/flushSync)). |
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.
Can we add some code here for a high-level view of what this looks like?
const dataPromise = {
then: () => {},
status: 'fulfilled',
value: `some data'
}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.
I don't think we want to show how to create a custom thenable.
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.
Yea while you can use a custom Thenable, it's probably recommended to just use a native Promise for this.
You can make it a sub-class like such:
class FulfilledPromise extends Promise {
status = 'fulfilled';
value = null;
constructor(data) {
super(resolve => resolve(data));
this.value = data;
}
}
const promise = new FulfilledPromise(data);This makes it more palatable for sticklers that thinks adding expandos to native objects is bad. It shows that the pattern is encouraged by the platform. If it wasn't, then why can I sub-class natives? If can't add fields, what am I supposed to do with those sub-classes?
But in practice when you're not adding any methods to the prototype this is really just the same thing as:
const promise = Promise.resolve(data);
promise.status = 'fulfilled';
promise.value = data;Which is smaller code and faster, so why not do that? It also doesn't risk getting compiled which would break native subclassing.
So maybe document one stickler version and one faster version (and hint that thenables might be even faster).
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.
done
| React will set the `status` field itself if the passed Promise does not have this field set. Suspense-enabled libraries should set the `status` field on the Promises they create to avoid unnecessary fallbacks. Calling `use` conditionally depending on whether a Promise is settled or not is discouraged. `use` should be called unconditionally so that React DevTools can show that the Component may suspend on data. | ||
| <Sandpack> |
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.
Can we do something like this instead:
And structure it so that the most relevant code (the promise creation) is what's visible first?
Like this you should see this:
With a Promise subclass
function getUser(id) {
return {
status: 'fulfilled',
value: `User #${id}`,
then: () => {}
}
}
function UserDetails() {
const user = use(getUser());
return <p>Hello, {user}!</p>;
}Without a Promise subclass
function getUser(id) {
return Promise.resolve(`User #${id}`);
}
function UserDetails() {
const user = use(getUser());
return <p>Hello, {user}!</p>;
}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.
done
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.
I like this!
Many examples in the docs that compares approaches like this tend to point out what to look for, like "Note how loading the users shows a loading state", but I don't think that's necessary here.
src/content/reference/react/use.md
Outdated
| ### Avoiding fallbacks by passing Promise subclasses {/*avoiding-fallbacks-by-passing-promise-subclasses*/} | ||
| React will read the `status` field on Promise subclasses to synchronously read the value without waiting for a microtask. If the Promise is already settled (resolved or rejected), React can read the value immediately without suspending and showing a fallback if the update was not part of a Transition (e.g. [`ReactDOM.flushSync()`](/reference/react-dom/flushSync)). | ||
| React will set the `status` field itself if the passed Promise does not have this field set. Suspense-enabled libraries should set the `status` field on the Promises they create to avoid unnecessary fallbacks. Calling `use` conditionally depending on whether a Promise is settled or not is discouraged. `use` should be called unconditionally so that React DevTools can show that the Component may suspend on data. |
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.
I think this is great content and very helpful!
It's a pretty advanced topic though so maybe point out the intended audience early? Maybe lead with:
If you are implementing a Suspense-enabled library, you can help React avoid unnecessarily suspending when you know the promise has already settled, by using
statusandvalueorreasonfields.
I think "subclass" and "microtask" is likely language that will trip people up, but if it's clear this is an advanced topic I think that's fine.
Maybe be a bit more explicit about the rejected/reason case as well?
Calling
useconditionally depending on whether a Promise is settled or not is discouraged.useshould be called unconditionally so that React DevTools can show that the Component may suspend on data.
I think this should be its own paragraph as it's a separate point.
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.
done
| React will read the `status` field on the Promise to synchronously read the value without having to wait for a microtask. If the Promise is already settled (resolved or rejected), React can read the value immediately without suspending and showing a fallback if the update was not part of a Transition (e.g. [`ReactDOM.flushSync()`](/reference/react-dom/flushSync)). | ||
| React will set the `status` field itself if the passed Promise does not have this field set. Suspense-enabled libraries should set the `status` field on the Promises they create to avoid unnecessary fallbacks. |
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.
I think this is good!
Maybe "... if the update was not part of a Transition (e.g. ReactDOM.flushSync())" is too much detail here? At least for me it made the sentence slightly harder to parse and didn't seem that important in this specific context, but not a big deal.
I wondered what it would look like if the docs tried to explain the basics more and noodled on a separate approach, I'm not at all sure this is better or captures everything correctly, so read it as an exploration:
| React will read the `status` field on the Promise to synchronously read the value without having to wait for a microtask. If the Promise is already settled (resolved or rejected), React can read the value immediately without suspending and showing a fallback if the update was not part of a Transition (e.g. [`ReactDOM.flushSync()`](/reference/react-dom/flushSync)). | |
| React will set the `status` field itself if the passed Promise does not have this field set. Suspense-enabled libraries should set the `status` field on the Promises they create to avoid unnecessary fallbacks. | |
| In JavaScript, Promise callbacks always run in a "microtask", even if the Promise has already settled. This means that if you call `use(alreadySettledPromise)`, React can not synchronously resolve its values and has to unnecessarily suspend. | |
| By adding `status: 'fulfilled' | 'rejected'` and either `value` or `reason` to the Promise when it settles, your library makes it possible for React to access these values synchronously and avoid suspending. It is recommended for Suspense-enabled libraries to do this. | |
| React will set these fields itself if the passed Promise does not have them set. |
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.
I like this, but I also like including examples of when this can happen. But I wouldn't use flush sync for that, I would use examples like "such as when the user clicks the back button, or when they continue interacting with the page while a transition is in progress"
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.
I like that. The preload case might be another one that's easy to understand and helps explain things.
| React will set the `status` field itself if the passed Promise does not have this field set. Suspense-enabled libraries should set the `status` field on the Promises they create to avoid unnecessary fallbacks. Calling `use` conditionally depending on whether a Promise is settled or not is discouraged. `use` should be called unconditionally so that React DevTools can show that the Component may suspend on data. | ||
| <Sandpack> |
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.
I like this!
Many examples in the docs that compares approaches like this tend to point out what to look for, like "Note how loading the users shows a loading state", but I don't think that's necessary here.
| // flushSync is only used for illustration | ||
| // purposes. A real app would probably use | ||
| // startTransition instead. | ||
| flushSync(() => { |
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.
NIT: Is flushSync necessary for this to happen, or is it there to illustrate the difference to a transition clearer?
If it's not necessary, it might distract from the point more than help, at least for me it made me wonder if this was a necessary condition?
I do think it's good to point out this does not happen with transitions though and not sure how to do that differently. 😄
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.
There's actually a much easier and common way for this to happen:
export default function App() {
const [userId, setUserId] = useState(null);
const [userUsable, setUser] = useState(null);
const [isPending, startTransition] = useTransition();
return (
<div>
<button
onClick={() => {
// ⚠️ this startTransition causes a sync update
// to App to update the `isPending` value
startTransition(() => {
setUser(preloadUser(1));
setUserId(1);
});
}}
>
Load User #1
</button>
{/* ... */}
<span>{isPending && 'loading...'}</span>
<Suspense key={userId} fallback={<p>Loading</p>}>
{/*
⚠️ Without proper memoization, the sync update
flows into this boundary, causing it to suspend
if the data is not available syncronously
*/}
{userUsable ? (
<UserDetails userUsable={userUsable} />
) : (
<p>No user selected</p>
)}
</Suspense>
</div>
);
}https://codesandbox.io/p/sandbox/ymm9s8?file=%2Fsrc%2FApp.js
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.
I realize now that you might both trying to show practical examples of how this situation might happen in real code? I was focused on just the practical aspects of what's necessary for it to trigger.
I was focused on the fact that even the first example triggers the suspend with just:
<button
onClick={() => {
setUser(preloadUser(2));
setUserId(2);
}}
>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.
Yeah that's also true, good point, the flushSync isn't necessary in any of the examples to trigger this.
However, not using a transition here would be an anti-pattern since it would force the fallback if the data wasn't synchronously available (i.e. preloading didn't finish), so I think using a transition is still good here.
https://react.dev/reference/react/Suspense#preventing-already-revealed-content-from-hiding
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.
not using a transition here would be an anti-pattern
Oh yeah, totally onboard with that.
so I think using a transition is still good here
I don't have a strong opinion on that, in my mind it all depends on what the purpose of the examples are, which I'm not a good judge of.
- Tightly focused on explaining the
use()behavior- Maybe better to keep them as minimal as possible to avoid distraction (but still point out in a comment this is an anti-pattern)
- Broader, try to explain common situations/why it's important etc
- Makes sense to show a more "complete" example
Personally, I already know there are a lot of situations this might happen in, so I lean towards minimal, but the broader examples might be more helpful to others!
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.
One point that's in favor of not having it is if you're using the Compiler, then the code works because everything is memoized correctly. But I still don't like the idea of synchronously setting the value your'e suspending on - you just shouldn't do that.
Instead, a better example would be if there was something in UserDetails that was idiomatically a sync update that needed to re-render, like a text input. Then that update wouldn't be in a transition, and you'd need to handle it correctly so it doesn't just suspend the text input you're typing in on every key press.
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.
Though in that case the Compiler also solves it :)
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.
The simplest case I can think of thats also common is likely just loading the page with prefetched data? Not sure if there is special behavior or if it makes for a good example though.
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.
If we are talking cases, wouldnt a transition where a new suspense boundary reveals with the use inside also have this problem? Fallback show if status is not set, never shows if it is?
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.
Correct. And actually I just noticed the key so this example is good as is (with or without the transition) so I think we should just leave the transition in?
Turning posts into docs
I.e. how React leverages
promise.status. Targeted at Suspense-enabled libraries.Preview