Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: decouple useEventCallback from the state management means #73

Open
leoyli opened this issue Aug 10, 2019 · 0 comments
Open

RFC: decouple useEventCallback from the state management means #73

leoyli opened this issue Aug 10, 2019 · 0 comments
Assignees

Comments

@leoyli
Copy link

leoyli commented Aug 10, 2019

Hi, thank you for the inspiration and make this awesome libs. I have the following RFC proposed:

Suggestion

Remove the overlapping of state management in useEventCallback, as we have useObservable, useState, and useReducer already, making this hook just purely for the async event management. This hook is best to be treated as an adaptor to convert DOMEvent to DOMEvent stream.

Why

After using it for a while, I always think useEventCallback is not so intuitive to use. It accepts up to 3 arguments: callback, initialState, and inputs. Why we need this initialState if we want to introduce prop dependencies? At the current implementation, what this hook would returns is not just a memorized event handler as a callback, but a tuple of handler + the observed state, which makes me feel this hook is doing too much, IMHO.

Ideally, what I have in mind is to have a similar interface as in the the native useCallback. But what can make this hook so useful is to convert the event object to an observable. Then we can manage DOMEvent as a stream easily.

If we need to performance side effects, like to update state, we should use tap operator instead. For example:

const deps = /* some dependencies, could be state, or props */
const [state, setState] = useState();

const onClick = useEventCallback(
  (event$, deps$) => event$.pipe(
    filter((e) => e.type === 'click'),
    map(({ target }) => target.value),
    tap((v) => setState(v))    // * re-render explicitly requested by the user.
    withLatestFrom(deps$),
    /* do something with dependencies */
  ),
  [deps] // optional
)

The deps would be any props or local state dependencies that is reactively emitted and passed as the second props to the callback. And we can think this like an epic in redux-observable. This way we decouple the state management part out of this hook. And this hook is never going to trigger re-render by itself, it will have to be introduced by the user.

Implementation

Here is the suggested implementation:

const createEvent$ = <T>() => {
  return new Subject<T>();
};

const createDeps$ = <D extends unknown[]>(deps: D) => {
  return deps.length ? new BehaviorSubject(deps) : undefined;
};

export const useEventCallback = <E = any, D extends unknown[] = never>(
  callback: (event$: Observable<E>, deps$: Observable<D>) => Observable<unknown>,
  deps: D = [] as never
) => {
  const event$ = useMemo(() => createEvent$<E>(), []);
  const deps$ = useMemo(() => createDeps$<D>(deps), []);

  useEffect(() => {
    const subscriber = callback(event$, deps$ as Observable<D>).subscribe();

    return () => {
      subscriber.unsubscribe();
      event$.complete();

      if (deps$) {
        deps$.complete();
      }
    };
  }, []);

  useEffect(() => {
    if (deps$) {
      deps$.next(deps);
    }
  }, deps);

  return useCallback((e: E) => event$.next(e), []);
};

And perhaps, it should be called as useEvent$Callback to make it more obvious what this hook is doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants