react

react copied to clipboard

Strict mode does not support the following code pattern:

export function useCache() {
	const params = useCacheParams()

	const cache = useRef<Cache>()
	if (cache.current == null) {
		cache.current = new Cache(params)
	}

	useEffect(() => {
		return () => cache.current!.close()
	}, [])

	return cache.current!
}

Cache will be created twice but closed only once.

The only possibility AFAIK is to initialize the cache in useEffect, but this is too slow because it won't be available on the first render, and too cumbersome as the users of the hook need to deal with the nullability.

I don't see anything wrong with this code pattern i.e. it does not assume anything that should not be assumed about React implementation, so it should be supported by strict mode.

I think the issue came from the null checks.

export function useCache() { const params = useCacheParams();

const cache = useRef<Cache | null>(null); if (cache.current == null) { cache.current = new Cache(params); }

useEffect(() => { return () => { if (cache.current) { cache.current.close(); } }; }, []);

return cache.current!; }

Same issue here, so let me provide some additional information:

Cache will be created twice but closed only once.

It was the instance of cache created by the second invocation (from StrictMode) that was being closed, so the first one never got the chance to cleanup.

Allow me to explain with a slightly different example (written in TSX):

class SomeUtility {

  private static id = 0
  private readonly id: number = SomeUtility.id++
  private isDisposed = false

  constructor() {
    console.log(`TestClass created [id: ${this.id}]`)
  }

  dispose = () => {
    if (this.isDisposed) {
      throw new Error(`Attempted to dispose but SomeUtility [id: ${this.id}] has already been disposed`)
    }
    this.isDisposed = true
    console.log(`TestClass disposed [id: ${this.id}]`)
  }

  greet = () => {
    if (this.isDisposed) {
      throw new Error(`Attempted to greet but SomeUtility [id: ${this.id}] has already been disposed`)
    }
    console.log(`Hello world [id: ${this.id}]`)
  }

}

function useCustomHook(utility: SomeUtility): void {
  useEffect(() => {
    utility.greet()
  }, [utility])
}

let renderCount = 0
function TestComponent(): JSX.Element {
  const utilityRef = useRef<SomeUtility>(null)
  if (utilityRef.current === null) {
    utilityRef.current = new SomeUtility()
  }
  useEffect(() => {
    return () => { utilityRef.current.dispose() }
  }, [utilityRef])
  useCustomHook(utilityRef.current)
  console.log(`renderCount: ${++renderCount}`)
  // Console results:
  // ```
  //   TestClass created [id: 0]
  //   renderCount: 1
  //   TestClass created [id: 1]
  //   renderCount: 2
  //   Hello world [id: 1]
  //   TestClass disposed [id: 1]
  //   Uncaught Error: Attempted to greet but SomeUtility [id: 1] has already been disposed
  //   Uncaught Error: Attempted to dispose but SomeUtility [id: 1] has already been disposed
  // ```
  // ❌ TestClass [id: 0] was never disposed until the end
  // ❌ TestClass [id: 1] which should be used is being disposed instead
  return null
}

Still, this problem has been around for very long (at least one year before this thread has been opened) and I think it's a legitimate concern. This issue deserves more attention.

Edit: It seems like #26315 also mentions of this problem.

This is not a bug in StrictMode but a bug in the code, which StrictMode catches.

@glyph-cat in your example, if you turn off StrictMode, and run the app it will work initially, but if you try to change the file so that it Fast Refreshes, you'll get the same error: https://codesandbox.io/p/sandbox/determined-austin-8kqjnk

This is because there are features in React that allow components to unmount effects but preserve the state of the component, then when the component mounts again, only the effects need to be recreated. This is why Effects need to be symmetrical, so that things that are created are destroyed, and things that are destroyed are created.

To fix this example, you must set the ref to null in destroy, and re-create the utility in create:

if (utilityRef.current === null) {
  utilityRef.current = new SomeUtility();
}

useEffect(() => {
  if (utilityRef.current === null) {
    utilityRef.current = new SomeUtility();
  }

  return () => {
    utilityRef.current.dispose();
    utilityRef.current = null;
  };
}, []);

Notice that with this change, the component doesn't error during either Fast Refresh, or in StrictMode: https://codesandbox.io/p/sandbox/peaceful-moon-vfdf6c

Note, I also needed to update useCustomHook to accept the ref instead of ref.current.

Here's some docs for more info:

• https://react.dev/learn/synchronizing-with-effects#how-to-handle-the-effect-firing-twice-in-development
• https://react.dev/reference/react/StrictMode#fixing-bugs-found-by-re-running-effects-in-development

This is not a bug in StrictMode but a bug in the code, which StrictMode catches.

@glyph-cat in your example, if you turn off StrictMode, and run the app it will work initially, but if you try to change the file so that it Fast Refreshes, you'll get the same error: https://codesandbox.io/p/sandbox/determined-austin-8kqjnk

This is because there are features in React that allow components to unmount effects but preserve the state of the component, then when the component mounts again, only the effects need to be recreated. This is why Effects need to be symmetrical, so that things that are created are destroyed, and things that are destroyed are created.

To fix this example, you must set the ref to null in destroy, and re-create the utility in create:

if (utilityRef.current === null) {
  utilityRef.current = new SomeUtility();
}

useEffect(() => {
  if (utilityRef.current === null) {
    utilityRef.current = new SomeUtility();
  }

  return () => {
    utilityRef.current.dispose();
    utilityRef.current = null;
  };
}, []);

Notice that with this change, the component doesn't error during either Fast Refresh, or in StrictMode: https://codesandbox.io/p/sandbox/peaceful-moon-vfdf6c

Note, I also needed to update useCustomHook to accept the ref instead of ref.current.

Here's some docs for more info:

* https://react.dev/learn/synchronizing-with-effects#how-to-handle-the-effect-firing-twice-in-development

* https://react.dev/reference/react/StrictMode#fixing-bugs-found-by-re-running-effects-in-development

@rickhanlonii The first render still creates an object that will never be disposed, because the first useEffect call happens after the rerender. Is there any way to dispose the first object?

In most cases this shouldn't be a problem in development, but I can see perhaps some objects may have active listeners that will never clean up.

@jacobtipp yeah StrictMode is catching a separate bug, which is that your component isn't pure. There's a side effect of mutating the id variable in render.

The solution is to not mutate in render. If this is an expensive resource, you also shouldn't be creating it in render because React may render multiple times before a component mounts (or may never mount at all) and you'll leak instances.

For example, say a child in this tree suspends, and the Suspense boundary is above this component. Then this component may render multiple times before the effect fires, creating multiple SomeUtility instances that are not cleaned up. If the user navigates away while you're suspended, then the component never mounted, so the effect cleanup won't fire and the resource won't be destroyed. So this should really be done in an effect, when you know the component has mounted.

@jacobtipp yeah StrictMode is catching a separate bug, which is that your component isn't pure. There's a side effect of mutating the id variable in render.

The solution is to not mutate in render. If this is an expensive resource, you also shouldn't be creating it in render because React may render multiple times before a component mounts (or may never mount at all) and you'll leak instances.

For example, say a child in this tree suspends, and the Suspense boundary is above this component. Then this component may render multiple times before the effect fires, creating multiple SomeUtility instances that are not cleaned up. If the user navigates away while you're suspended, then the component never mounted, so the effect cleanup won't fire and the resource won't be destroyed. So this should really be done in an effect, when you know the component has mounted.

A situation I'm dealing with is similar to this example but I need to create a resource and provide it to a subtree of components using React.Context (a Provider component that creates a disposable instance of a class and provides it to all children). If I create the resource in a useEffect I'm forced to either return an empty fragment until the resource is created, or return the context.Provider with null as its initial value. This is not ideal because consumers of this context are expecting a resource that they can use and subscribe to.

I was assuming I could create the resource in a ref like in this React docs example:

https://react.dev/reference/react/useRef#avoiding-recreating-the-ref-contents

I'm guessing I don't have any options but to create the resource in useEffect since you say a component may not mount at all even after rendering multiple times.

@rickhanlonii any comment on my original example? I don't see anything wrong with it. The problem seems to be with StrictMode not supporting this pattern.

This is not a bug in StrictMode but a bug in the code, which StrictMode catches.

@glyph-cat in your example, if you turn off StrictMode, and run the app it will work initially, but if you try to change the file so that it Fast Refreshes, you'll get the same error: https://codesandbox.io/p/sandbox/determined-austin-8kqjnk

This is because there are features in React that allow components to unmount effects but preserve the state of the component… (truncated)

This solution seems to work, at least in a way that doesn't causes error and it feels like a safe way to me. But like @jacobtipp mentioned, the first instance (id: 0) will never be disposed.

But since the effects only run once in production, I guess it's fine for now. The only thing left that worries me is how one can make similar code work with future features such as off-screen rendering when it becomes an official thing, but that is a problem for another day I guess.