devexperts / dx-platform

Mozilla Public License 2.0
33 stars 24 forks source link

react-kit: useObservable considered to be unsafe #213

Closed Nedgeva closed 4 years ago

Nedgeva commented 4 years ago

Hi folks!

Consider following example (nevermind the fact that wrapping function into observable is generally bad practice and should be avoided if possible, but it's still real-case scenario):

const useObservable = dxUseObservable(instanceObservable);
const constVoid = () => {};

const sample = of(() => console.log("i'll never get called"));

export default function App() {
  const fn = useObservable(sample, constVoid);

  // boom!
  // fn is undefined
  fn();

  return (
    <div className="App">
      <h1>Hello CodeSandbox</h1>
      <h2>Start editing to see some magic happen!</h2>
    </div>
  );
}

( PoC can be found at codesanbox playground: https://codesandbox.io/s/frosty-platform-musl7 )

using function as value passed to observable (and to initial too) causes useState under the hood of useObservable util to run it to get the actual value according to: https://reactjs.org/docs/hooks-reference.html#lazy-initial-state So it's kinda unsafe and may lead to runtime exceptions.

Suggested solution: wrap initial value and every next one into constant thunk.

raveclassic commented 4 years ago

we need to use react lazy state initialisation together with state modification instead of setter

Nedgeva commented 4 years ago

@raveclassic if we're going to keep both function-as-value and lazy initializers then perhaps we need 3rd argument (maybe optional) in useObservable that indicates that function is initializer, otherwise treat 'em as ordinary value.

raveclassic commented 4 years ago

We'd want to avoid any unnecessary API bloat. For rxjs it would be the following:

import { Observable } from 'rxjs';
import { useEffect, useMemo, useState } from 'react';

export const useObservable = <A>(fa: Observable<A>, initial: A): A => {
    const [value, setValue] = useState(() => initial);
    const subscription = useMemo(
        () =>
            fa.subscribe(a => {
                //ignore state toggle functions and allow passing functions in Observable
                setValue(() => a);
            }),
        [fa, setValue],
    );
    useEffect(() => () => subscription.unsubscribe(), [subscription]);
    return value;
};

Note that this allows you to pass Observable<Function> safely. See specific calls to useState and setValue

raveclassic commented 4 years ago

fixed #218