facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.14k stars 46.89k forks source link

Feature request: A useLayoutEffect with read/write batching across a tree #20068

Open mattgperry opened 4 years ago

mattgperry commented 4 years ago

Today, useLayoutEffect can be used for synchronous read/writes across the DOM.

useLayoutEffect(() => {
  // Write
  ref.current.style.transform = ""

  // Read
  const box = ref.current.getBoundingClientRect()
})

For a single instance of a single component, this works well. But if this code is repeated or reused anywhere in the tree, we trigger layout thrashing. The severity of the layout thrashing scales linearly with the number of hooks/components featuring either this code or code like it.

Hooks and components are designed to be composable, yet it's this trivial to write one that isn't.

Instead, what I'd like is a batched version of useLayoutEffect that provides read and write callbacks. These schedule callbacks that will be called:

  1. Synchronously before paint
  2. In "parallel" in reads/writes/reads etc
  3. After all child components in the tree have run useLayoutEffect - including those entering the tree (currently these don't mount until after other useLayoutEffects have been called)

It could look like this, though I'm more interested in the above specs than actual API:

useBatchedLayoutEffect((read, write) => {
  write(() =>  {
    ref.current.style.transform = ""
  })

  read(() => {
    const box = ref.current.getBoundingClientRect()
  })
})

Then, adhering the 3 specifications above, these callbacks are executed in order, so all reads from across the tree, then all writes, then all reads etc. There is no upper limit for the number of permitted ping-ponged reads/writes IMO as the amount of layout thrashing you could possibly suffer will never be worse than the single hungriest hook. In my experience I've never needed more than a read/write/read/write.

Measurement accuracy

In the given example, we're measuring a component after first resetting its transform because we want to snapshot its actual bounding box without any transforms applied. But if this component is nested in itself, so both a parent and child are performing the same type of animation, we want to unset all the transforms before measuring any of the elements, otherwise the resulting measurements will be incorrect.

What about requestAnimationFrame?

Theoretically it could be possible to implement this ourselves in userland by creating a batcher that executes on the next available frame. Sadly this isn't possible in practise. For whatever browser-internal reason it is necessary to run these reads and writes synchronously to prevent flashes of incorrectly-styled components.

If you play with this very simple layout animation implementation by clicking on the red box you'll see it suffers no flashes:

https://codesandbox.io/s/broken-star-cycfz?file=/src/App.js:528-537

But if you uncomment the wrapping requestAnimationFrame within the useLayoutEffect you'll see it does randomly flash with the undesired styles.

Andarist commented 4 years ago

It would be great if you could prepare demos for each use case you'd like to cover with this that would help illustrate how the proposal can make implementing it possible (or easier). The currently provided codesandbox doesn't really showcase the problem because "it works" - it only includes a single animation for a single component and the whole idea behind the proposal is to make it easier to coordinate animations across the tree.

mattgperry commented 4 years ago

@Andarist Here's a demo that demonstrates both the problems I'm suffering: https://2qiyw.csb.app/

By running the profiler you can see style recalculations getting triggered multiple times rather than just once: image

Likewise you can see how when nested, child components can't schedule themselves to be measured until after all parent transforms have been reset.

gaearon commented 4 years ago

There was a related proposal: https://github.com/reactjs/rfcs/pull/96

https://github.com/reactjs/rfcs/blob/customhostnode-rfc/text/0000-custom-host-node.md

Would this solve the problem for you?

Andarist commented 4 years ago

@gaearon that RFC seems to be concerned with taking control over the whole node in a more imperative manner but still synced to the React's lifecycle. The @mattgperry's proposal is different - it's much more about the reads/writes orchestration throughout the tree. The fact that animations have to manage style attribute imperatively is a secondary concern here.

To better illustrate the problem I've implemented a demo implementing the proposal in userland: https://codesandbox.io/s/confident-sun-umfj6?file=/src/App.js . It might differ in some aspects - that's acceptable and some semantics can be refined in this implementation. It's also possible that some proposed semantics won't be possible to implement using currently available APIs (that would sway the argument in favor of exposing new APIs). This implementation also requires an additional wrapper node to orchestrate things and operates on a sync update from within the layout effect - the goal of this proposal is to eliminate the need for both of those.

markerikson commented 4 years ago

I'm curious about the desired execution ordering of these callbacks.

If a parent and its two children all call this hook and provide callbacks, what order would/should they be run in?

(Very vaguely related: one of our issues on React-Redux has always been that commit-phase lifecycles run bottom-to-top, which means that a new parent that immediately renders new children can result in the children subscribing to the store before the parent. If there was an alternate lifecycle that ran top-to-bottom for the entire tree, we might be able to use that instead.)

Andarist commented 4 years ago

I'm curious about the desired execution ordering of these callbacks.

@markerikson take a look at the sandbox I've provided above - it has been made after discussing this with @mattgperry in private, there is a chance that I've screwed something up, but it should be pretty much close to his vision. Note that I haven't thought much about at what times those should be called in relation to other existing effects because this is currently out of my control in the userland - by removing the constraint of the wrapper node this would change because React would actually have to decide when to call those, for now, the descendants schedule read/writes in their layout effects and they are flushed within a layout effect of the wrapper node after a scheduled state update is flushed by React (which should always be synchronous in here because of the synchronous setState from within a layout effect)

If a parent and its two children all call this hook and provide callbacks, what order would/should they be run in?

In what order read/writes should be executed? Well, it doesn't quite matter - we can't quite forbid what actually people do in within those scheduled operations but they have a very specific purpose related to animations. It doesn't matter if you read stuff from [siblingA, siblingB, parent] or any permutation of this because the world should not be changed between those reads so, at the end of the day they should be somewhat idempotent (just during the operations flush), the same principle applies for writes.

If there was an alternate lifecycle that ran top-to-bottom for the entire tree, we might be able to use that instead.

That would be probably a separate proposal - I mean this idea here maybe could be abused to do what you want (if the decided order of calls would be different from the one implemented in my sandbox), but as a concept, this is a variation of existing useLayoutEffect so I kinda would expect that this would not satisfy your use case and it would probably have a similar constraint when it comes to calling this on the server (but I guess you actually just use isomorphic layout effect or smth to work around this so probably same could be done about this one).

mattgperry commented 4 years ago

@gaearon That RFC would be more pertinent to our discussion the other day, where I was suggesting components would need to apply animated styles via useLayoutEffect. That would at least allow that work to be performed before DOM-reading useLayoutEffects fire, and this is something I am stuck on in terms of readying Framer Motion for concurrent mode. It's related but separate.

As @Andarist explained, here I'm more interested in fixing layout thrashing across a whole tree, batching read/writes in a way that is synchronous before paint.

A more generic alternative that would allow me to write something like this in userland is a lifecycle hook that runs after all useLayoutEffect hooks in a tree. This way all the components wanting this functionality could schedule the read/writes in useLayoutEffect and then, in a useAfterLayoutEffect or whatever flush everything that has been scheduled:

useLayoutEffect(() => {
  myAPI.schedule((read, write) => {
    read(() => {})
    write(() => {})
  })
})

useAfterLayoutEffect(() => {
  myAPI.flush()
})

You couldn't do a similar pattern with the linked RFC and flush in useLayoutEffect because new components to the tree won't have mounted yet.

@markerikson I don't think I have a use-case where this is important to be honest but I assume they get triggered FIFO

mattgperry commented 3 years ago

I've been taking another look at this as layout thrashing has become a problem in Framer.

I've setup this Sandbox which is a simplified example of some batching code I'm trying to implement in Framer Motion: https://codesandbox.io/s/pensive-shadow-wnt8l?file=/src/Layout.js

This illustrates the use-case. I reiterate, I'm attempting to batch two lifecycle methods across the React tree, getSnapshotBeforeUpdate and componentDidUpdate/Mount.

In this simplified example both first need to reset transforms across the tree, then both need to measure bounding boxes throughout the tree.

I think that the batching of both is entirely possible in user land if there was even just an API that allowed us to know when a component was the last of a group to perform a certain lifecycle event. Without componentWillUpdate I feel like this is impossible.

mattgperry commented 3 years ago

Here's a kind of minimal reproduction about the kind of contracts I need https://gist.github.com/mattgperry/c44b573dc16444b5e9a0e3e60604cba5