mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.94k stars 32.27k forks source link

`useSynchronousEffect` is not React-Hot-Loader compatible #15999

Closed theKashey closed 5 years ago

theKashey commented 5 years ago

Source issue - https://github.com/gaearon/react-hot-loader/issues/1262

One of the biggest issues for React-Hot-Loader in the last past months were hooks, which were not working properly, especially from the hot-loading point of view.

Version 4.9.0 introduced the fix for it via adding one more argument to every hook with dependencies, like useEffect.

The Problem

useSynchronousEffect is split into two parts - apply the effect, and remove it.

function useSynchronousEffect(func, values) {
  const ref = React.useRef([]); // THIS REF WOULD BE PRESERVED
  let output;

  // THIS WOULD NOT BE RE-RUN ON HOOK UPDATE
  if (ref.current.length !== values.length) {
    ref.current = values;
    output = func();
  } else {
    for (let i = 0; i < values.length; i += 1) {
      if (values[i] !== ref.current[i]) {
        ref.current = values;
        output = func();
        break;
      }
    }
  }

  React.useEffect(
    () => () => {
      if (output) {
        output();      //  WILL BE RESET ON EFFECT RE-RUN
      }
    },
    values, // eslint-disable-line react-hooks/exhaustive-deps
  );
}

This issue should be solved regardless of the nature of hooks hot reloading - a special logic like this should be a bit more reliable.

theKashey commented 5 years ago

Let me elaborate more on this:

PS: useState theoretically could be updated on HMR, but there is no way to update(not reset) useRef.

Solutions

1. "Key" based

function useSynchronousEffect(func, values) {
  const [key] = useRef({});
  let output;
  // store "generation" key. Just returns a new object every time
  const currentKey = useMemo(() => {}, values); // eslint-disable-line react-hooks/exhaustive-deps
  // "the first render", or "memo dropped the value"
  if(key.current !== currentKey){
    ref.current = currentKey;
    output = func();
  }

  React.useEffect(
    () => output, // there is no need of a complex construction here.
    [currentKey], // eslint-disable-line react-hooks/exhaustive-deps
    // ^ use the same variable here.
  );
}

1. Isomorphic

Use effect on SSR, and switch to useEffect on CSR.

const useSynchronousEffect = isServer ? (effect, props) => effect() : useEffect

The problem - there is no way to detect is this "Server" or "Not". It's not bound to the environment, but to the react command used to kick off the render. Probably it should be

function useSynchronousEffect(func, values) {
  const isServer = React.useContext(IsServer); // But still - who will set this value?
   isServer ? effect : useEffect(effect, values);
}
oliviertassinari commented 5 years ago

I'm looking into it.

Naismith commented 5 years ago

@oliviertassinari Hey Oliver, have you had any chance to look into this yet? This has been bugging me in the current project I am working on.

theKashey commented 5 years ago

I've doublechecked this behavior with upcoming React Fresh, and it would be the same. So we have to find a solution, compatible with these constraints, but I am not sure that MUI shall do it by their own, as long as any project might need that Synchronous Effect, in the same way MUI needs it.

oliviertassinari commented 5 years ago

@theKashey Thanks for checking! I haven't looked into it yet, to be honest, but I hope we can release a v4.1.1 tomorrow with a fix and the TypeScript icon regression fix. This hook logic also has an issue with the Concurrent Mode, the problem is related.

oliviertassinari commented 5 years ago

It's fixed in https://github.com/mui-org/material-ui/pull/16195 and will be soon released in Material-UI v4.1.1.