Batching makes it difficult to perform imperative actions like focus

[astoilkov]

React version: 16.9.5

Steps To Reproduce

1. Overwrite batched updates with the following code: ReactDOM.unstable_batchedUpdates = callback => callback()
2. Batched updates aren't actually overwritten.

Reasoning

I recognize that this may not be classified as bug because it isn't a documented feature but I have tried to search for a different solution but to no avail. Fixing this behavior can open a new way of using React. I tried writing on Stack Overflow and writing to @gaearon.

I have a number of arguments which support the disabling of batched updates in event handlers and in effects initialization. If anybody is willing to read a document and consider this scenario I am willing to write an RFC.

[astoilkov]

I started wondering if RFCs for changes to React isn't a better place to write this issue. Let me know if you agree and I will close this one and write one there.

[gaearon]

If anything, we are going to batch more by default, not batch less. Batching is important for multiple reasons, including drastically improving performance for bubbling events, and to avoid inconsistent trees.

If you need to make a specific call unbatched, you can do this manually:

ReactDOM.flushSync(() => {
  // this setState won't be batched
  setState(something)
})

[astoilkov]

Is there a way to apply flushSync() to every event handler? I can provide a custom useLayoutEffect() and useEffect() functions but it is harder with event handlers.

We constantly observe bugs caused by the same code behaving differently in two places because at one place batching is happening and at the other, it isn't. I am not saying disabling batching by default, I believe in general this is better for most projects but I can clearly see that in our project it is introducing bugs for the past year. This is strange because our software is almost bug-free. However, batching is probably our biggest contributor to bugs right now.

[gaearon]

Is there a way to apply flushSync() to every event handler?

No. (And you'd need to apply it to setState calls, not event handlers.)

We constantly observe bugs caused by the same code behaving differently in two places because at one place batching is happening and at the other, it isn't.

This is interesting. This behavior has been there for over five years, and this is the first time I'm hearing this worded so strongly. (Usually people learn about batching once, learn not to read from this.state, and call it a day.) So I'm curious if there are particular patterns that cause this to be an issue for you.

If it's the inconsistency that gets you, you could apply batchedUpdates in more places instead. In a future version of React, this will be the default behavior for everything so the inconsistency will go away.

[gaearon]

I guess if you really want to do it, maybe something like this could work:

const origSetState = React.Component.prototype.setState
React.Component.prototype.setState = function() {
  ReactDOM.flushSync(() => {
    origSetState.apply(this, arguments);
  })
}

and you can probably come up with something similar for Hooks (or use your own useState Hook).

Needless to say, this is terrible for performance and will cause issues both for third-party components and for the future you who will be debugging this.

[astoilkov]

I am currently writing an example to show you the cases we are hitting and why bugs are appearing. Will get back to you soon.

[astoilkov]

Here is the example: https://codesandbox.io/s/cranky-sanderson-7kdcx.

Now let me explain:

1. Clicking "Log in" throws an error showing the bug.
2. I know that this example can be fixed by adding autoFocus to the input. However, imagine that focusing is conditional and the component doesn't know if it should focus itself when being created. If the new component should be focused is determined from the place that triggers the new component to show so no way to know in the component itself. Adding if the component should be focused as a state property introduces a new set of problems that I can explain if you want.
3. The same happens with any method of the element like scrollIntoView() or getBoundingClientRect(). For us focus() and scrollIntoView() are the most common. Mainly focus().
4. I understand this is a domain-specific problem because our application is shortcut-heavy and manages focus very strictly. I can even show you the app and give you 4-5 bugs that were caused by that behavior.

The main problem is that inputRef.current in the onClick() handler will have a different value depending on batched updates being used or not.

Note that if you can't understand what are the use cases of having access to a ref after updating the state I can go into bigger details and real-world use cases in our app.

I hope that you understood me. Let me emphasize that our app manages focus a lot and React isn't good at handling focus. Maybe if a good solution for focusing exists this kind of problems can be fixed with an abstraction and not by disabling batched updates but until then I have tried many things and haven't found a solution.

[astoilkov]

By the way, using flushSync() we can do something else and avoid disabling batched updates directly. We have methods that change the state and we can wrap all of them with flushSync(). I am describing everything above in order to help you understand a set of use cases that I believe are valid. I hope I will be helpful because for me it is a little strange I didn't found anybody talking about this problem.

[gaearon]

Thanks for a clear example.

Let me emphasize that our app manages focus a lot and React isn't good at handling focus. Maybe if a good solution for focusing exists this kind of problems can be fixed with an abstraction and not by disabling batched updates but until then I have tried many things and haven't found a solution.

That's a good callout. It's fair to say React doesn't really give you the tools to do it very well. We've been doing some work in that area but it's behind an experimental flag and is not ready to be a stable API yet. This is definitely something that's on our minds but we won't have a solution in the very near future.

In class-based code, you could do this to work around the problem:

setState({ isVisible: true }, () => {
  this.inputRef.curent.focus()
})

That's the idiomatic workaround in classes. We don't (yet?) have something similar exposed for Hooks because that exact API won't work. In particular, it would "see" old props and state which would be highly confusing.

Currently, in function components the only way you can get "notified" about commits is with useLayoutEffect. Here is an example:

  useLayoutEffect(() => {
    if (isTwoFactorAuthenticationVisible) {
      inputRef.current.focus();
    }
  }, [isTwoFactorAuthenticationVisible]);

That would solve your problem: https://codesandbox.io/s/laughing-fast-9785u.

However, I agree this solution isn't ideal because the desire to focus might in some cases have more to do with which action triggered it rather than which state changed.

I think there are at least two opportunities for RFC design here:

• More intuitive and declarative focus management that works within the React paradigm. (There was some work on this here: RFC: Focus Management API reactjs/rfcs#109, although I don't think this necessarily addresses your concerns. Maybe you could comment on it.)
• A way to express "run some side effect after a particular setState is flushed". We have this for classes, but not for Hooks. It seems like a useful feature. But it's not clear what the design should be like. So working on that would also be beneficial.

I don't think the solution is to "revert back" to a paradigm where each setState is atomic. We think this would be a regression. Instead, we think this needs to be "fixed forward" by finding the right abstractions that work with the React model. In the meantime, I hope the workarounds above are reasonable.

[gaearon]

I reopened this for discussion with a different title.

[gaearon]

Here's a small abstraction you could build in userland today:

  const perform = useCommand(command => {
    if (command === "focusInput") {
      inputRef.current.focus();
    }
  });

  // ...

  setState(...)
  perform("focusInput');

https://codesandbox.io/s/blissful-elgamal-umn3p

In this example, useCommand maintains an internal queue of commands to run at the commit time. You can schedule more of them by calling perform returned to it. The commands are guaranteed to run in the commit phase and each runs only once.

Unlike my earlier example (https://codesandbox.io/s/laughing-fast-9785u), note that this only sets the focus on every click, even if the state didn't change. This seems to match your intent of correlating this with the user action rather than with the state change.

Don't know if this is sufficient for this use case but maybe at least for a subset?

[gaearon]

cc @sophiebits I think you had something similar for useEffect

[gaearon]

@astoilkov Can you tell me more about your use cases that don't have to do with focus? You mentioned measurements. Some simplified but relatively realistic code would help here.

[astoilkov]

• flushSync() works, I will definitely experiment with it.
• this.setState() doesn't because our codebase is written with Hooks.
• The useLayoutEffect() isn't ideal as you described. In our app, exactly which action triggered the show matters.
• useCommand() – very interesting, let me sleep on it, I think it is a good alternative.

Let me give you an example not related to focus. I will describe it as making a code example will be complicated. BTW if you don't understand the example I can make a GIF showcasing the use case in our app.

• We have a Files Sidebar which shows the files of the project. Just like any modern code editor.
• Our app by default has an auto reveal feature. Just like VSCode Explorer: Auto Reveal option. Opening a file scrolls to it in the Files Sidebar using element.scrollIntoView().
• Now imagine you have folderA/fileA.md and folderB/fileB.md. You move from fileA to fileB. The Files Sidebar hasn't still rendered fileB.md and should wait for the render to happen in order to call scrollIntoView().
• This call to scrollIntoView() happens only when opening files but not at other times. Exactly as you said – which action was triggered matters.

function openTab() {
  // call Redux or other global store for changing the file state

  // access the Files Sidebar ref to the fileB item and call `scrollIntoView()`
}

Regarding the RFC: I read it few months ago when I was struggling with finding a good implementation for calling focus() from one component into another. It indeed doesn't solve this problem but I will go through it again and see if I can point to this issue as a reference describing that it can be worth considering it.

I agree the solution isn't to revert back.

Your comments made me think. I will reflect on our conversation and I will write here again.

Thank you for the time spent considering this.

[astoilkov]

https://codesandbox.io/s/objective-rgb-lb0cu

I made a new example so we can discuss a more real-world scenario. Our app has global shortcuts for opening tabs. Opening a tab should focus the editor. How do we focus the newly created editor? After we know how to do it, how do we make it work when batching updates?

I am aware that we can again use autoFocus or useLayoutEffect() but this focusing when the editor is created is optional in our app and depends on the reason why the tab will be opened – previewing a file or opening it for edits.

[astoilkov]

I started understanding why you wanted me to give you an example for getting measurements. Because getBoundingClientRect() returns a value which you then want to use and your useCommand() idea won't work then.

We don't have an example with getBoundingClientRect() or anything that returns a value in our app. I can imagine a theoretical example – but I don't like those, we should look at real-world examples.

function openTab() {
  // change state

  // this is harder
  const rect = elementRef.current.getBoundingClientRect()
}

This can be solved with a similar technique to useCommand() it just needs a little different API:

function openTab() {
  // change state

  nextLayoutEffect(() => 
    const rect = elementRef.current.getBoundingClientRect()
  })
}

[gaearon]

We chatted about this with @sebmarkbage and he noticed these examples tend to be similar to our internal notion of “update queues”. So maybe that’s something we could expose as a lower level concept.

It seems like they often have a particular thing in common: you want “the last one to win”. For example:

• If multiple things call focus, you want only the last one to win.

• If multiple things call scrollIntoView, you want the last one to win.

However these queues are separate. You don’t want scrollIntoView to “overwrite” the focus operation.

Note that declarative attributes like autoFocus suffers from a flaw where instead of the last operation winning, we have the last sibling winning. That doesn’t really make sense. It’s the last operation that matters.

We were thinking there could something be:

import { queueFocus } from 'react-dom'

const onClick = () => {
  queueFocus(inputRef)
  setState()
})

Note we’re passing the ref itself. So it doesn’t matter that React hasn’t set it yet. It will read from mutable object when the time comes.

If it’s a more generic concept that’s not limited to focus then it could get more powerful. But also more verbose.

// singleton
export const useScrollQueue = createEffectQueue()
export const useFocusQueue = createEffectQueue()

// app
import { useFocusQueue } from 'somewhere'

// ...
const queueFocus = useFocusQueue()

const onClick = () => {
  queueFocus(inputRef)
  setState()
})

Although then the question becomes where do we keep those queues and how to get third party components to agree on using them. So perhaps they should be built-in after all.

This is why we want to understand the full breadth of the use cases. Are they all “fire and forget”, are they all “last one wins”, and how many common distinct ones there are.

Maybe this could be an RFC.

[gaearon]

With focus specifically @trueadm said there’s even more pitfalls. That there are cases to consider about blur event handlers causing other focus, or focus causing other focus. Cascading effects. And that sometimes the browser needs to paint first. I don’t have all the background knowledge to talk about this but it’s something you would need to research if you plan to write an RFC.

[astoilkov]

Hmm...does scrollIntoView() last call wins? Imagine two scrollable containers with overflow: scroll; set to them. They both can take calls and scroll individually.

I have some ideas for solutions. Give me some time to implement and think about them.

[astoilkov]

I haven't tested this fully but what do you think about this:

import { useMemo, MutableRefObject, useRef, useCallback } from 'react'
import useForceUpdate from 'use-force-update'

export default function useElementRef<T extends HTMLElement | null>(): MutableRefObject<T> & {
  queueFocus(): void
  queueScrollIntoView(arg?: boolean | ScrollIntoViewOptions | undefined): void
} {
  const nextLayoutEffect = useNextLayoutEffect()
  const value = useMemo(() => {
    const callback: MutableRefObject<T> & {
      queueFocus(): void
      queueScrollIntoView(arg?: boolean | ScrollIntoViewOptions | undefined): void
    } = (element: T): void => {
      callback.current = element
    }
    callback.current = null!
    callback.queueFocus = (): void => {
      nextLayoutEffect(() => {
        callback.current?.focus()
      })
    }
    callback.queueScrollIntoView = (arg?: boolean | ScrollIntoViewOptions): void => {
      nextLayoutEffect(() => {
        callback.current?.scrollIntoView(arg)
      })
    }

    return callback
  }, [nextLayoutEffect])

  return value
}

function useNextLayoutEffect(): (callback: () => void) => void {
  const callbacks = useRef<(() => void)[]>([])
  const forceUpdate = useForceUpdate()

  return useCallback(
    (callback: () => void) => {
      callbacks.current.push(callback)
      forceUpdate()
    },
    [forceUpdate],
  )
}

Usage:

const inputRef = useElementRef()

function openTab() {
  // change state

  inputRef.queueFocus()
}

[gaearon]

Hmm...does scrollIntoView() last call wins? Imagine two scrollable containers with overflow: scroll; set to them. They both can take calls and scroll individually.

That's a good point!

[astoilkov]

I want to explain why I am searching for alternatives to useFocusQueue(). If the developer doesn't understand this problem they will, without researching, call ref.current.focus(). Even if they know about useFocusQueue() there are still two problems:

• Why will you use it instead of .focus()? You don't understand the reason, you don't use it.
• How you don't forget about using it? How a developer unfamiliar with the problem know to use it?

Unfortunately, my solution doesn't solve these problems. It solves two smaller problems:

• Using TypeScript I can Omit<HTMLElement, 'focus'> and the compiler will complain when you try to call ref.current.focus(). (Edit: I am experimenting with this and it doesn't seem such a good idea.)
• Set a standard for using useElementRef() for elements ref which will make it one step easier to try to call focus(), fail and then see the queueFocus() method.

[gaearon]

How you don't forget about using it? How a developer unfamiliar with the problem know to use it?

ESLint can help here. Same as with other patterns that aren't recommended.

[astoilkov]

'no-restricted-syntax': [
  'error',
  {
    selector: 'MemberExpression[property.name="focus"][object.property.name="current"]',
    message: `Don't call focus() directly on an element. Use queueFocus() instead.`,
  },
]

[astoilkov]

Based on our discussion I wrote a document outlining the benefits and disadvantages of all the possible solutions you and I came up with. Based on that document I implemented one of the solutions in our project (a variation of the things we have discussed).

If the experiment is stable for some period of time I will post the solution here.

[callmetwan]

@astoilkov I wanted to follow up on this discussion, how did your experiment turn out? Would you mind to share that document here so we can all learn from your experience?

[astoilkov]

@callmetwan Yes. We have been testing the solution in our production app for a few months and everything looks great. I am working on an open-source repo and will share it when it's done(2-3 weeks from now).

[astoilkov]

Here is the solution we have been using in production for a few months without it causing a single bug – https://gist.github.com/astoilkov/5d7b493634586a48d2f1c336349af9d8.

I tried to open-source a repo but there are a lot of questions I will need to answer first.

P.S. The gist doesn't provide implementations for queueSelect() and queueScrollIntoView() but if @gaearon likes the queueFocus() solution I can update the gist with more information and implementation for the other two hooks.

[omarabid]

How do you prevent batching for Redux props updates?

[BrandonWilliamsCS]

@astoilkov It looks like you'll need to consider "remounting" behavior before upgrading to React 18 (at least when using strict mode). Related blog section.

In particular, I suppose you'd need to set isUnmountedRef.current = false in the body of that first useLayoutEffect?

[astoilkov]

Yep. You are right. I will need to make some changes in order to support React 18. I've started working on a general-purpose solution as well. However, I haven't tested it in React 18 which is something that I'm planning when we decide to upgrade to React 18.