uppy icon indicating copy to clipboard operation
uppy copied to clipboard

`@uppy/react` is not Compatible with `React.StrictMode`

Open rart opened this issue 3 years ago • 2 comments

Initial checklist

  • [X] I understand this is a bug report and questions should be posted in the Community Forum
  • [X] I searched issues and couldn’t find anything (or linked relevant results below)

Link to runnable example

https://codesandbox.io/s/nifty-fire-7luyeb

Steps to reproduce

When using React.StrictMode the Dashboard doesn't render. To reproduce, wrap your app or component containing the Dashboard in <React.StrictMode>. See codesandbox above.

Expected behavior

Dashboard — and other components — works regardless of StrictMode

Actual behavior

The Dashboard renders an empty square without most functionality.

Most of this may be due to the usage of class components in @uppy/react.

rart avatar Aug 01 '22 17:08 rart

This might be because in dev-mode, React with StrictMode will mount every component twice to reveal any bugs related to shared state and race conditions not handled by frequent mount/unmount. We should definitely fix this, because async rendering needs this to work.

mifi avatar Aug 29 '22 11:08 mifi

FWIW the bug is also reproducible on the 3.x branch: https://codesandbox.io/s/musing-blackwell-rpqhxr

aduh95 avatar Sep 09 '22 19:09 aduh95

Are there any workarounds for this?

d-pollard avatar Sep 26 '22 16:09 d-pollard

I think a workaround is to selectively enable strict mode in other places in your app, except for Uppy. See https://github.com/facebook/react/issues/16362

mifi avatar Sep 27 '22 08:09 mifi

I rewrote a hook which follows a recommended pattern from this discussion https://github.com/reactwg/react-18/discussions/18 I've successfully tested it with React.StrictMode but the drawback is that it would be a breaking change as the uppy instance is no longer returned directly by the hook (and it was bad according to the new react docs, https://beta.reactjs.org/learn/referencing-values-with-refs#best-practices-for-refs, "don't read or write ref.current during rendering").

I made a codesandbox to compare the current useUppy which make the app fails at some point to retrieve the correct uppy instance and a new implementation compliant with StrictMode.

https://codesandbox.io/s/useuppy-c6688u?file=/src/Unsafe.tsx

Start by clicking Render button which will log an error that the TestPlugin is no longer defined (that is the instance has been closed)

The new hook code (written in TypeScript) would be

import Uppy from "@uppy/core";
import { useEffect, useRef } from "react";

export const useSafeUppy = (factory: () => Uppy) => {
  const ref = useRef<Uppy>();
  const getterRef = useRef(() => {
    if (ref.current === undefined) {
      ref.current = factory();
    }
    return ref.current;
  });
  useEffect(() => {
    return () => {
      if (ref.current !== undefined) {
        ref.current.close();
        ref.current = undefined;
      }
    };
  }, []);
  return getterRef;
};

Stouffi avatar Sep 29 '22 12:09 Stouffi

Thanks for investigating this! I'm inclined to introduce a breaking change if we fix strict mode and at the same time get rid of a bad practice.

Murderlon avatar Sep 29 '22 13:09 Murderlon