-
Notifications
You must be signed in to change notification settings - Fork 48.8k
Add Suspensey Images behind a Flag #32819
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
Conversation
Since we no longer diff all props in the render phase we might still get here when nothing has changed. So this adds a small optimization to avoid resuspending on every update.
Timeout at 500ms which is the same timeout we use for fonts in transitions but maybe we should wait less by default.
…nd Idle Meaning the blocking ones and Offscreen doesn't suspend on the images. We should allow an opt-in to suspend blocking too in the future. Resources still suspend blocking.
530f531
to
4b430b6
Compare
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.
LGTM! Very easy to review, exciting when features like this land in such relatively small diffs
Since we have tests for this in noop.
// We indicate that all images are not yet loaded and if they're able | ||
// to hit cache we let the decode() do that. Even if we did maintain | ||
// our own cache to know this, it's not a guarantee that the browser | ||
// keeps it in decoded memory. |
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 consequence of this is that we end up showing fallbacks for a Suspense boundary in a Transition even if we've preloaded it before revealing it (which its is throttled in its reveal).
We might want to add a heuristic here in case it is synchronously available to let it commit anyway. Even though that can lead to flashes due to images not being decoded.
The Suspense optimization could help here because we could yield and see if we can eagerly solve it in a micro-task however that could force decoding a bit too early which gives opportunity for the decoding cache to get dropped before commit.
I think instead, what might be ideal is to detect if an image is already loaded and if so, only mark the commit as might suspend but not suspend the Suspense boundary. I think I'll do that.
…mplete We will still visit this in the commit phase to decode() it in case it's not decoded already.
We've known we've wanted this for many years and most of the implementation was already done for Suspensey CSS. This waits to commit until images have decoded by default or up to 500ms timeout (same as suspensey fonts). It only applies to Transitions, Retries (Suspense), Gesture Transitions (flag) and Idle (doesn't exist). Sync updates just commit immediately. `<img loading="lazy" src="..." />` opts out since you explicitly want it to load lazily in that case. `<img onLoad={...} src="..." />` also opts out since that implies you're ok with managing your own reveal. In the future, we may add an opt in e.g. `<img blocking="render" src="..." />` that opts into longer timeouts and re-suspends even sync updates. Perhaps also triggering error boundaries on errors. The rollout for this would have to go in a major and we may have to relax the default timeout to not delay too much by default. However, we can also make this part of `enableViewTransition` so that if you opt-in by using View Transitions then those animations will suspend on images. That we could ship in a minor. DiffTrain build for [efb22d8](efb22d8)
We've known we've wanted this for many years and most of the implementation was already done for Suspensey CSS. This waits to commit until images have decoded by default or up to 500ms timeout (same as suspensey fonts). It only applies to Transitions, Retries (Suspense), Gesture Transitions (flag) and Idle (doesn't exist). Sync updates just commit immediately. `<img loading="lazy" src="..." />` opts out since you explicitly want it to load lazily in that case. `<img onLoad={...} src="..." />` also opts out since that implies you're ok with managing your own reveal. In the future, we may add an opt in e.g. `<img blocking="render" src="..." />` that opts into longer timeouts and re-suspends even sync updates. Perhaps also triggering error boundaries on errors. The rollout for this would have to go in a major and we may have to relax the default timeout to not delay too much by default. However, we can also make this part of `enableViewTransition` so that if you opt-in by using View Transitions then those animations will suspend on images. That we could ship in a minor. DiffTrain build for [efb22d8](efb22d8)
<details> <summary>React upstream changes</summary> - facebook/react#32821 - facebook/react#32819 - facebook/react#32816 - facebook/react#32815 - facebook/react#32812 - facebook/react#32762 - facebook/react#32808 - facebook/react#32807 </details>
[diff facebook/react@040f8286...33661467](facebook/react@040f828...3366146) <details> <summary>React upstream changes</summary> - facebook/react#32823 - facebook/react#32822 - facebook/react#32825 - facebook/react#32821 - facebook/react#32819 - facebook/react#32816 - facebook/react#32815 - facebook/react#32812 - facebook/react#32762 - facebook/react#32808 - facebook/react#32807 </details>
[diff facebook/react@040f8286...33661467](facebook/react@040f828...3366146) <details> <summary>React upstream changes</summary> - facebook/react#32823 - facebook/react#32822 - facebook/react#32825 - facebook/react#32821 - facebook/react#32819 - facebook/react#32816 - facebook/react#32815 - facebook/react#32812 - facebook/react#32762 - facebook/react#32808 - facebook/react#32807 </details>
We've known we've wanted this for many years and most of the implementation was already done for Suspensey CSS. This waits to commit until images have decoded by default or up to 500ms timeout (same as suspensey fonts).
It only applies to Transitions, Retries (Suspense), Gesture Transitions (flag) and Idle (doesn't exist). Sync updates just commit immediately.
<img loading="lazy" src="..." />
opts out since you explicitly want it to load lazily in that case.<img onLoad={...} src="..." />
also opts out since that implies you're ok with managing your own reveal.In the future, we may add an opt in e.g.
<img blocking="render" src="..." />
that opts into longer timeouts and re-suspends even sync updates. Perhaps also triggering error boundaries on errors.The rollout for this would have to go in a major and we may have to relax the default timeout to not delay too much by default. However, we can also make this part of
enableViewTransition
so that if you opt-in by using View Transitions then those animations will suspend on images. That we could ship in a minor.