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

Move responsibility of cleaning up effects from facets to useEffect() / useFacetEffect() #96

Merged
merged 7 commits into from
Oct 24, 2022

Conversation

AdamRamberg
Copy link
Contributor

@AdamRamberg AdamRamberg commented Oct 18, 2022

Background

I had a scenario where a useFacetEffect() (effect) depended on a facet from a useFacetMap(). I noticed that the clean up handler for the effect was sometimes called, but the effect itself wasn't called again with the new value(s). This introduced an issue to me since I was subscribing / unsubscribing to an event in the effect / clean up.

Problem

After investigating the issue I noticed that this occurs because of how the facet itself is responsible of cleaning up listeners (effects). useFacetMap() is furthermore just a thin layer on top of the facet adding a selector and an equal check. If an effect is subscribing to the useFacetMap() facet and the underlying facet updates, but the useFacetMap() is not (because of its equality check), the effect's clean up function will run, but not the effect itself with new value(s).

Solution

This PR removes the functionality of keeping track of clean up handlers in the facet itself. That logic is instead moved to useFacetEffect() / useFacetLayoutEffect(). This accomplishes 2 things:

  • Effects and corresponding clean up handlers are called as expected.
  • Decreases the complexity of regular facets and make them more lightweight than before.

As a bonus, this PR adds a factory function that creates the useFacetEffect() and the useFacetLayoutEffect() hooks.

Full list of changes

  • Remove cleanup logic in createFacet.ts
  • Re-implement cleanup logic in useFacetEffect()
  • Create a factory for creating useFacetEffect() and useFacetLayoutEffect()
  • Exclude /dist folder when running unit tests
  • Added integration test to cover the problem described in this PR

- Re-implement cleanup logic in useFacetEffect()
- Create a factory for creating useFacetEffect() and useFacetLayoutEffect()
- Exclude /dist folder when running unit tests
@AdamRamberg AdamRamberg changed the title Fix effect on use facet map Move responsibility of cleaning up effects from createEffect to useEffect / useFacetEffect Oct 18, 2022
@AdamRamberg AdamRamberg changed the title Move responsibility of cleaning up effects from createEffect to useEffect / useFacetEffect Move responsibility of cleaning up effects from createFacet() to useEffect() / useFacetEffect() Oct 18, 2022
@AdamRamberg AdamRamberg changed the title Move responsibility of cleaning up effects from createFacet() to useEffect() / useFacetEffect() Move responsibility of cleaning up effects from facets to useEffect() / useFacetEffect() Oct 18, 2022
@AdamRamberg AdamRamberg marked this pull request as ready for review October 18, 2022 12:41
Copy link
Member

@pirelenito pirelenito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Freaking awesome that we get to simplify the createFacet implementation.

Copy link
Member

@pirelenito pirelenito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awaiting QA review before we can merge this.

Copy link
Member

@pirelenito pirelenito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a go from QA!

@AdamRamberg AdamRamberg merged commit ec2ea3b into main Oct 24, 2022
@AdamRamberg AdamRamberg deleted the fix-effect-on-use-facet-map branch October 24, 2022 06:48
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

Successfully merging this pull request may close these issues.

2 participants