davidkpiano / useeffectreducer Goto Github PK

Thinking out loud: we need some way to control the cleanup of effects. Right now, useEffectReducer treats effects as fire-and-forget, which is fine for many use-cases, but can introduce memory leaks or undesired behaviors for certain effects that are supposed to have a specific lifetime.

One solution: exec(effect) can return an entity which can be explicitly managed in the reducer state:

const someReducer = (state, event, exec) => {
  if (event.type === 'FETCH') {
    return {
      // ...
      effect: exec({ type: 'fetch', user: 'David' })
    }
  else if (event.type === 'CANCEL') {
    // stop effect
    state.effect.stop();

    return {/* ... */}
  }
}

The effect returned from exec(...) will be an opaque object that (for now) just has a unique id and a .stop() method. This will call the disposal function returned from the effect:

const [state, dispatch] = useEffectReducer(someReducer, initialState, {
  fetch: () => {
    // ...

    return () => {
      // cancel fetch request
    }
  }
});

The module entry in package.json is:
https://github.com/davidkpiano/useEffectReducer/blob/master/package.json#L56

  "module": "dist/useEffectReducer.esm.js",

However the built file is:

dist/use-effect-reducer.esm.js

thereby breaking usage in rollup

the whole library wants to do something like this

function useEffectReducer(notReducer, initialState) {
  const [state, setState] = useState(initialState);
  effects
  const dispatch= (action) => {
    setState(notReducer(state, action, useEffect))
  } 
  return [state, dispatch];
}

but the React will raise error for this piece of code, cause you should not use hooks this way,
so we can add all functions with effect inside a list

function useEffectReducer(notReducer, initialState) {
  const [state, setState] = useState(initialState);
  const effects = useRef([])
  const exec =  (a) => effects.current.push(a);

  const dispatch= (action) => {
    setState(notReducer(state, action, exec))
  } 
  useEffect(() => {
      effects.current.forEach(a => a())
      effects.current = []
  }, [state])

  return [state, dispatch];
}

so now we can run our effects anywhere we want.
what is wrong with this code? say we animate something using jQuery inside our reducer then calling exec inside it or calling an API then after that call exec when the data received,
so you implement a library that everywhere in the codebase you can call useEffect.

do you know how much effort the React team has done so you can't use effects this way?

I think it is better that every action or side effect will be wrapped inside a Task monad or something like that so the reducer will return a tuple of state and effects

also, the reducer is not pure as I said in twitter and you blocked me, I hope you understand that you are an influencer in this community and saying something that is not correct make other people like me in a mistake.

nevertheless, I'm sorry if my behavior was not appropriate I was just wanted to inform you and others like me that our reducer function is not pure.

Hi! This implementation has a bug that can cause effects to be permanently skipped.

The special flush event clears all pending effects from the queue:

This seems fine because event is dispatched after clearing all effects in the queue:

The problem is that this effect closes over stateEffectTuples. It's possible that new effects will be added to the queue in between the render and the execution of the effect. These effects won't be executed in the effect because of the closure, but they will be cleared by the flush event. So they will effectively be skipped.

Here's a sandbox demonstrating the problem. Click "increment" and notice that the state correctly updates to 2, but the console has only printed INC 0. We'd expect it to also print INC 1. The latter effect was skipped because the second increment was added to the queue after the closure but before the dispatch of the flush event. So the effect gets added to the queue and then cleared without having a chance to run.

One potential solution would be to slice off the first stateEffectTuples.length elements from the state at the time of the update rather than unconditionally returning []. Essentially this is saying "flush the effects that were just run" rather than "flush all effects".

I'm wondering if it's ok to call dynamic dependencies within effects:

const history = useHistory()
const queryClient = useQueryClient()

const [state, dispatch] = useEffectReducer(reducer, initialState, {
  invalidateUsers: () => queryClient.invalidateQueries("users"),
  redirectToHome: () => history.push("/")
})

Let's suppose that history and queryClient will keep changing between rerenders, is my service implementation going to work?

Forked from: https://twitter.com/meijer_s/status/1252252222500622336?s=20

I think it makes sense to pass dispatch to the effect's arguments as well, so that we don't need to declare our effects, or "api" within the scope of the component.

I believe this would also benefit the test-ability of the effects. As I could now just test api and stub dispatch with jest.fn.

const api = {
  fetchDog: async (state, effect, dispatch) => {
    try {
      const data = await fetchRandomDog();
      dispatch({ type: 'RESOLVE', data });
    } catch (error) {
      dispatch({ type: 'REJECT', error });
    }
  },
};

function dogReducer(state, event, exec) {}
const initialState = {};

function DogFetcher() {
  const [state, dispatch] = useEffectReducer(dogReducer, initialState, api);

  // ...
}

Hey @davidkpiano !

First of all, thank you for this awesome reducer! It's the perfect approach for me and my team right now without getting 100% into xstate (yet).

Anyways, we are planning on upgrading to react 18 soon and I would like to know if I can count on not having effects running multiple times (due to the whole Strict Mode thing).
Don't know if you have tested useEffectReducer with it yet, but if you did, I was hoping to know your results 🙏

Thank you for your time!

Would it be possible to expose the prevState to our effects as well?

This would allow us to move the decision to run expensive side effects to the effect, instead of having it in the reducer, wrapping the exec call.

Hello! First off, love this library so much, it's made my code so much more clean.

Simplified reproduction: https://codesandbox.io/s/red-https-bxpo0?file=/src/App.tsx

But anyways, I've run into this scenario recently and was able to resolve by resolving this locally - but, to be honest, I'm not really sure of the React internals enough to PR this + test. Anyways, the scenario

1. I want to use useEffectReducer in my own hook and export out it's dispatch so that other components can programmatically send out events to the reducer.
2. However, when I leverage the dispatch it emits events twice.
3. To resolve, you can useMemo the wrappedReducer .

const memoReducer = useMemo(() => wrappedReducer, []);

const [[state, effectStateEntityTuples, entitiesToStop], dispatch] = useReducer(
    memoReducer,
    initialStateAndEffects
 );

Hello,

I've noticed that the last update to this package was some 3 years ago and that was to the README, with any other change outside of this occurring 4 years ago. Given this and issues being raised such as React 18 support I'm wondering if there are any planned updates or improvements? Currently it looks as if the package is not being maintained.

Thanks!

In order to see actions in redux devtools.