-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
POC: Transition compatibility hooks #44151
base: master
Are you sure you want to change the base?
Conversation
8e67af4
to
b324cfe
Compare
@mnajdova (cc @romgrk, @Janpot) this should be decent enough for eyes now. I added an example for one way this might be integrated into the For more primitive components, the right answer isn't as clear cut. As a general rule of thumb, actions (and transitions) should be used for interactions that might make the UI unresponsive and/or trigger undesirable loading states (e.g. by causing existing suspense boundaries to revert to their fallback). It would be impossible for a framework like MUI to make this determination on behalf of its users. Therefore, my recommendation would be to:
For other components, I would simply strive to help users understand how and when to use transitions and the related APIs by including examples of their use in the documentation, and generally educating them that they should attempt to minimize the amount of work done directly in low-level event handlers like |
b324cfe
to
26a4109
Compare
@@ -24,6 +24,8 @@ import { useDefaultProps } from '../DefaultPropsProvider'; | |||
import autocompleteClasses, { getAutocompleteUtilityClass } from './autocompleteClasses'; | |||
import capitalize from '../utils/capitalize'; | |||
import useSlot from '../utils/useSlot'; | |||
import { unstable_useDeferredValueCompat } from '@mui/utils'; |
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.
import { unstable_useDeferredValueCompat } from '@mui/utils'; | |
import useDeferredValueCompat from '@mui/utils/useDeferredValueCompat'; |
And tbh I'd even drop the "compat" from the name. The fact that it's a compat shim is an implementation detail, just like use-sync-external-store
is; there's no need to carry that information where the hook is used.
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.
What do you think of the useReducer
/useState
case? I can drop the Compat
suffix there too, but then it's not clear that it's related (and necessary). Maybe useStateWithTransitions
? 🤷♂️ Let me know your thoughts!
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 I prefer carrying that semantic rather than "compat".
@@ -422,6 +424,7 @@ const AutocompleteGroupUl = styled('ul', { | |||
|
|||
export { createFilterOptions }; | |||
|
|||
const EMPTY_ARRAY = []; |
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 really wish the spec would add something like Array.EMPTY
, we keep adding empty values like this all over the place.
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.
we keep adding empty values like this all over the place.
We could add it in our utils package? Not sure there would be a noticeable benefit 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.
Not sure there would be a noticeable benefit though
It avoids useless memory allocations, and in some situations it does make an impact. A lot of the perf PRs I did in the last months were just removing memory allocations.
We could add it in our utils package
I usually do that, and it's frequent in other codebases as well, e.g. https://github.com/styled-components/styled-components/blob/main/packages/styled-components/src/utils/empties.ts
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'm sure you'll notice a difference avoiding allocations in frequently called code. but we're talking about a single allocation at module scope in a few modules vs. allocating once and importing it. The code a bundler generates to import it from a utils package is probably more overhead than just allocating it in the module itself. I wouldn't be surprised if that styled-components
snippet you share is actually slower than what we have here. Your proposal to make it part of the language makes a lot more sense to me.
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 code a bundler generates to import it from a utils package is probably more overhead than just allocating it in the module itself.
Depends on the bundler, some of them generate a single scope where they just give unique names to all variables including imports/exports so "importing" something in prod is basically just using a variable (IIRC, vite does it), while others do a require
-like snippet of code to replace imports (IIRC, webpack).
LGTM overall, though I've skimmed through it because I haven't touched this part of the codebase yet. Do you have a showcase of how it improved Autocomplete? Note that you can get a codesandbox with |
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.
In general the proposal looks good to me. I left some minor comments. I would like to test a bit more the Autocomplete component's changes.
packages/mui-utils/src/CompatTransitionManager/CompatTransitionManager.ts
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,104 @@ | |||
import * as ReactDOM from 'react-dom'; |
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.
Very clean implementations 👌
packages/mui-utils/src/useDeferredValueCompat/useDeferredValueCompat.ts
Outdated
Show resolved
Hide resolved
* TODO: Improve typing. | ||
*/ | ||
function useReducerCompat<T>(reducer: any, initializerArg: any, initializer?: any) { | ||
if (typeof React.startTransition === 'function') { |
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.
We can likely do this condition outside of the hook and just return the regular React.useReducer
if the startTransition
function exists in React.
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'm not really sure, to be honest 🫤 Consider:
startTransition(() => {
dispatch('foo');
});
dispatch('bar');
If we use React.useReducer
directly even in React 18, then the behavior of dispatch
could change significantly depending the installed version of React. Specifically, state (or reducers) that would produce blocking updates in React 17 (because they don't use the shim, and we can't queue updates that don't use the shim) would suddenly produce non-blocking updates in React 18 (because React can queue them).
By using the shim even in React 18, we ensure that the behavior is always the same regardless of the React version: shimmed useTransition
will only ever cause shimmed useState
/useReducer
updates to be non-blocking. Non-shimmed useState
/useReducer
updates are always blocking, even if changed inside of a shimmed useTransition
in React 18: instead, you have to intentionally switch to the non-shimmed useTransition
to change the behavior.
At least, those are my thoughts. If you feel particularly strongly about it though, we can change it.
* Like `useState`, but for use with `useTransitionCompat`. | ||
*/ | ||
function useStateCompat<T>(initialValue: T | (() => T)): [T, SetStateFn<T>] { | ||
if (typeof React.startTransition === 'function') { |
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.
Same as the previous comment
packages/mui-utils/src/useDeferredValueCompat/useDeferredValueCompat.ts
Outdated
Show resolved
Hide resolved
2ebae08
to
52d57ce
Compare
52d57ce
to
052caf8
Compare
@@ -24,6 +24,8 @@ import { useDefaultProps } from '../DefaultPropsProvider'; | |||
import autocompleteClasses, { getAutocompleteUtilityClass } from './autocompleteClasses'; | |||
import capitalize from '../utils/capitalize'; | |||
import useSlot from '../utils/useSlot'; | |||
import { useDeferredValue } from '@mui/utils'; | |||
import Fade from '../Fade'; |
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.
This would be inconsistent with the Select animation, no?
import Fade from '../Fade'; |
I guess it's also outside of the scope of the problem we go after?
* If a batch doesn't already exist, a new one will be created, and the given | ||
* callback will be executed when it ends. | ||
*/ | ||
export function runWithBatch<T>(fn: () => T, batchCallback: BatchCallback): T { |
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.
Could we make transitions opt-in? Something that only React 19 users can adopt, and hence have no ponyfill?
export function runWithBatch<T>(fn: () => T, batchCallback: BatchCallback): T { |
@@ -572,6 +575,9 @@ const Autocomplete = React.forwardRef(function Autocomplete(inProps, ref) { | |||
className: classes.paper, | |||
}); | |||
|
|||
const groupedOptionsDeferred = useDeferredValue(popupOpen ? groupedOptions : EMPTY_ARRAY); |
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.
This should be in the hook version, no? So "Base UI" users can have the same benefit
const groupedOptionsDeferred = useDeferredValue(popupOpen ? groupedOptions : EMPTY_ARRAY); |
Proof of concept of compatibility hooks enabling transitions to be used in React 17 applications. There are a few key differences outlined below.
Compatibility Hooks
unstable_useDeferredValueCompat(value)
Implements
React.useDeferredValue
. Note that the React 19initialValue?
argument is not currently supported, as there are open questions as to shimming this behavior in React 18.unstable_useTransitionCompat()
Implements
React.useTransition
, including support for async actions. Note that the semantics for this hook differ slightly from React. Consider the following:It's expected that the value of
state
will be['foo', 'bar']
. That's because conceptually, state updates are queued, and even though the first update was scheduled non-blocking, the update still needs to be applied chronologically before the second update. Unfortunately, this type of queuing is not trivial to implement generally (i.e. for all state) without brittle monkey patching of React.Instead, the compatibility hook works a little differently. Instead of marking all state updates inside of
startTransition
as non-blocking, it marks everything as blocking by default -- even in React 18 and higher. To mark state updates as non-blocking with the compatibility hook, the state hook itself must use eitherunstable_useStateCompat
orunstable_useReducerCompat
. That means in order to reproduce the React 18 behavior, you need to write this instead:Once ready to adopt React 18, you would first migrate
unstable_useTransitionCompat
touseTransition
, and thenunstable_useStateCompat
/unstable_useReducerCompat
alternatives.unstable_useStateCompat
Just like
useState
, but for use withunstable_useTransitionCompat
. If you're not updating the value in a transition, you should just useuseState
.unstable_useReducerCompat
Just like
useReducer
, but for use withunstable_useTransitionCompat
. If you're not updating the value in a transition, you should just useuseReducer
.How it works
The
CompatTransitionManager
maintains batches, queues of updates that should be applied together. The primary question is when the batches should be applied:Whenever a blocking update is queued (i.e. outside of
startTransition
oruseDeferredValue
), all previous batches (even non-blocking) must be applied before the blocking update is. This is implemented byblockingBatchCallback
Whenever a non-blocking update is queued, the update can be applied at some point in the future. This is implemented by
nonBlockingBatchCallback
, which simply usessetTimeout
to apply the changes in a future macrotask, so blocking updates have a chance to be painted first.Running the compatibility hooks in React 18 is a special case, as we want to preserve the same semantics for those upgrading from React 17. This is implemented by
createPassthroughBatchCallback
, which executes only updates batched via the compatibility hooks inside the actualstartTransition
.React has a much more sophisticated strategy than these compatibility hooks. However, without being able to interrupt or reprioritize rendering work, it just doesn't make much sense to be more complex than this. We accept that without incremental rendering, non-blocking rendering will inevitably make the UI less responsive (that's why it was marked as non-blocking in the first place: so it wouldn't make the UI less responsive), and so we just make sure we paint any pending states/loading indicators in a blocking update, and cram as much work as possible into the subsequent update.
Why?
Transitions help make applications more responsive and improve Core Web Vitals like INP, especially on lower end devices like mobile. While the compatibility hooks aren't perfect, they can significantly improve the user experience.
Consider the
Autocomplete
component. Without transitions, running with 20x slowdown, you end up with something like this:Screen.Recording.2024-10-16.at.11.35.42.AM.mov
The UI simply freezes while the large list is being rendered. However, by using
useDeferredValue
, we can achieve this instead:Screen.Recording.2024-10-16.at.11.26.18.AM.mov
Even in React 17 via
unstable_useDeferredValue
we can achieve a significant improvement to INP (~800ms vs 2s). While the UI is unable to quickly respond to subsequent interactions while the dropdown content is rendering (due to the lack of time slicing/incremental rendering prior to React 18), we are still able to show our animation and a loading indicator to the user.(Note: the changes to autocomplete are not implemented in this PR. In reality, we would likely want a browser controlled animation that only starts after e.g. a 400ms delay, so that users on faster devices don't see it).
Ideally, applications would just be able to use React 18 or higher. However, in cases where that's not feasible, they should still ideally use transition and action semantics and APIs.