Skip to content
This repository has been archived by the owner on Apr 3, 2024. It is now read-only.

Add support for cleaning up effects. #8

Merged
merged 7 commits into from Apr 23, 2020
Merged

Add support for cleaning up effects. #8

merged 7 commits into from Apr 23, 2020

Conversation

davidkpiano
Copy link
Owner

This PR adds support for cleaning up effects by introducing "effect entities", which are objects that are returned from exec(...) and hold the current state of an effect:

  • EntityStatus.Idle if the effect hasn't been executed yet
  • EntityStatus.Started if the effect has been executed
  • EntityStatus.Stopped if the effect has been disposed

An effect's disposal function can now be returned from the effect implementation, just as in useEffect:

// ...

exec(() => {
  const sub = something.subscribe();

  return () => { sub.unsubscribe(); }
});

// ...

The difference between how effects are disposed in React and how they are disposed in useEffectReducer is that the developer has full control over the "lifecycle" of the effect, by maintaining a reference to that effect entity:

  • exec(effect) queues the effect for starting (in useEffect()) and returns an effect entity
  • exec.stop(entity) queues the effect represented by the entity for disposal
// In the effect reducer:

// ...
if (event.type === 'START') {
  return { 
    ...state,
    sub: exec(() => {/* ... */}
  }
}

if (event.type === 'STOP') {
  // queues the effect for disposal
  exec.stop(state.sub);

  return state;
}
// ...

Additionally, all running effects will be automatically disposed when the component is unmounted.

Closes #1

@davidkpiano davidkpiano mentioned this pull request Apr 22, 2020
@Ephem
Copy link

Ephem commented Apr 22, 2020

Just to see that I have read this PR correctly, you are using three separate useEffects in the implementation:

  • One to dispose any effect that has been queued to stop
  • One to run any effect that has been queued
  • One to dispose all effects on component teardown

So unlike how one uses useEffect that assumes a cleanup is always necessary if values change so the new effect can run, in this library, effects are only disposed explicitly by the user as the response to a dispatch (or on teardown), which pushes some responsibility to the user:

  • Stop/dispose existing fetches before making a new one
  • Stop timeouts before queuing a new one
  • Track which props/state-changes must result in a new dispatch, to do the above (probably in a useEffect that dispatches when they change)

The last one is one that users got wrong a lot of the time in the lifecycle-based paradigm, which led to this new "sync-state-to-sideeffects"-model that flushes out bugs and edge cases early. That's also a part of why it is rough to use.

You wrote on Twitter:

Instead of doing dependency-array juggling to figure out the effect lifecycle, you have direct control of it

This doesn't mean that complexity has gone away though, only that it has moved. I'd almost like to reformulate that into:

Instead of doing dependency-array juggling to figure out the effect lifecycle, you are encouraged to(/must) model your state and transitions in such a way that it makes more explicit when long lived effects are supposed to start and stop

I'd say these recent tweets by Sebastian Markbåge aligns pretty well with this approach, just that in this library, this happens in the reducer in a (hopefully) modelled way, which I think is nice.

While I think there might definitely be nuances and pitfalls to this approach which might make it easy to introduce bugs in some cases if you don't model things correctly, that exists with useEffect too and can be covered with documentation and patterns. (I'm thinking of things like not relying on closed over props from outside the reducer in exec for example, instead dispatching them in the event.)

While I don't have a clear enough mental model of Concurrent Mode yet to judge the approach or implementation from that perspective (and I do worry..), it looks good to me! 💯

@davidkpiano
Copy link
Owner Author

Let's take the timeout example. What really is stopping a timeout before starting a new one, in this case? To me, it seems like replacing an effect.

As an enhancement, we can have exec.replace(oldEntity?, newEffect) so that the timer example becomes:

// start timer
return {
  ...state,
  timer: exec(/* setTimeout */)
}

// restart timer
return {
  ...state,
  timer: exec.replace(state.timer, /* setTimeout */)
}

That would be preferable, I suppose, to this (more explicit):

// restart timer
exec.stop(state.timer);
return {
  ...state,
  timer: exec(/* setTimeout */)
}

What do you think?

@Ephem
Copy link

Ephem commented Apr 22, 2020

I like that! Not so much because the API is nicer (it is), but also because its existance encourages the user to think about those cases and make sure they are handled. "Why would I need to replace an effect? Oh, right."

I guess the one worry I'm having a hard time articulating with the API is that it seems very simple on the surface, but requires some specific patterns to avoid bugs. It kind of looks like a more high level API than it really is, but I think that can be mitigated by docs (esp patterns!) and perhaps warnings?

Would a warning when you are execing a parameterized effect that is already running be helpful in some cases?

Could you detect in dev when a running entity in the state was replaced by a new one without first stopping the old one?

I'm just thinking out loud. 😀

@Ephem
Copy link

Ephem commented Apr 22, 2020

A concrete example of what I mean is that your current dog example looks very reasonable to me. I dont intuitively understand that the fetchDog-effect works like an actual effect and can get stale, needs stopping/replacing etc and has a race condition.

I think for some people, who might not read the docs, the lure of this library could be to avoid that complexity entirely and move to the simpler (but buggier) mental model of the past. That's not a reason it shouldnt exist or isnt a nice abstraction, just pointing out a perceived challenge.

Perhaps this library could evolve to get it's own lint-rules someday? 🤔

@davidkpiano
Copy link
Owner Author

A concrete example of what I mean is that your current dog example looks very reasonable to me.

I'll merge this in as-is (after docs, etc.) and update the example.

I think for some people, who might not read the docs, the lure of this library could be to avoid that complexity entirely and move to the simpler (but buggier) mental model of the past.

There's footguns with either approach. It's way too easy to forget to dispose effects with useEffect() alone.

@davidkpiano davidkpiano merged commit 5b40a5c into master Apr 23, 2020
@davidkpiano davidkpiano deleted the davidkpiano/1 branch April 23, 2020 21:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Effect cleanup
2 participants