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

CONTRIBUTING.md

@@ -1,7 +1,8 @@
-# Bumping versions
+# Creating a new release
 
 Currently the process of updating the version of the packages is mostly manual. Before you start, first you need to make sure you have the permissions to publish the packages.
 
+- If there is a release branch (see __Release candidate__ below) that hasn't been merged to `main` yet now is the time to do so.
 - Make sure that you are logged in into your `npm` account. Use the command `yarn npm login` on the project folder to do this.
 - While on the `main` branch.
 - Perform a search an replace on all "package.json" files from the old version to the new. (ex `0.1.4` to `0.2.0`).
@@ -12,4 +13,16 @@ Currently the process of updating the version of the packages is mostly manual.
 - Publish the packages by running `yarn publish` (it will also build the packages).
 - Then head to GitHub to draft a new release https://github.com/Mojang/ore-ui/releases/new
 - Choose the tag and click the button "Auto-generate release notes"
-- Click "publish release"
+- Click "publish release" 🚀
+
+# Release candidate
+
+- Make sure that you are logged in into your `npm` account. Use the command `yarn npm login` on the project folder to do this.
+- Create a branch including the changes for the release candidate and name the branch to the same thing as the upcoming release, eg: `v0.4`.
+- Perform a search an replace on all "package.json" files from the old version to the new. (ex `0.3.12` to `0.4.0-rc.0`).
+- Run `yarn` to update the lockfile.
+- Commit the changes with a message containing the new version (ex: `0.4.0-rc.0`).
+- Create an annotated git tag by running `git tag -a v0.4.0-rc.0` (replace with the version). The tag message can be the version again.
+- Push commit and the tag `git push --follow-tags`.
+- Publish the packages by running `yarn publish --tag rc` (it will also build the packages).
+- We are currently not doing release notes for release candidates so you are all done! 🎉

jest.base.config.js

@@ -11,4 +11,5 @@ module.exports = {
     '^.+\\.jsx?$': 'babel-jest',
     '^.+\\.tsx?$': 'babel-jest',
   },
+  testPathIgnorePatterns: ['/node_modules/', '/dist/'],
 }

packages/@react-facet/core/src/facet/createFacet.spec.ts

@@ -219,23 +219,19 @@ describe('cleanup', () => {
 describe('setWithCallback', () => {
   it('prevents calling listeners if a setter returns NO_VALUE', () => {
     const facet = createFacet({ initialValue: 10 })
-    const cleanupMock = jest.fn()
-    const listenerMock = jest.fn().mockReturnValue(cleanupMock)
+    const listenerMock = jest.fn()
 
     facet.observe(listenerMock)
 
     // after observing it, the listener is called once with the initial value (but not the cleanup)
     expect(listenerMock).toHaveBeenCalledTimes(1)
     expect(listenerMock).toHaveBeenCalledWith(10)
-    expect(cleanupMock).not.toHaveBeenCalled()
 
     listenerMock.mockClear()
     listenerMock.mockClear()
-    cleanupMock.mockClear()
     facet.setWithCallback(() => NO_VALUE)
 
     // after using a setter callback to return NO_VALUE, the previous cleanup should be called, but not the listener again
     expect(listenerMock).not.toHaveBeenCalled()
-    expect(cleanupMock).toHaveBeenCalledTimes(1)
   })
 })

packages/@react-facet/core/src/facet/createFacet.ts

@@ -1,12 +1,6 @@
 import { defaultEqualityCheck } from '../equalityChecks'
 import { Cleanup, EqualityCheck, Listener, WritableFacet, StartSubscription, Option, NO_VALUE } from '../types'
 
-interface ListenerCleanupEntry {
-  cleanup: Cleanup
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  listener: Listener<any>
-}
-
 export interface FacetOptions<V> {
   initialValue: Option<V>
   startSubscription?: StartSubscription<V>
@@ -24,7 +18,6 @@ export function createFacet<V>({
   let currentValue = initialValue
   let cleanupSubscription: Cleanup | undefined
 
-  let listenerCleanups: ListenerCleanupEntry[] = []
   const checker = equalityCheck?.()
 
   const update = (newValue: V) => {
@@ -51,22 +44,8 @@ export function createFacet<V>({
 
     currentValue = newValue
 
-    if (listenerCleanups.length !== 0) {
-      for (let index = 0; index < listenerCleanups.length; index++) {
-        listenerCleanups[index].cleanup()
-      }
-
-      // start with a new array
-      listenerCleanups = []
-    }
-
     for (const listener of listeners) {
-      const cleanup = listener(currentValue)
-
-      // if the listener returns a cleanup function, we store it to call latter
-      if (cleanup != null) {
-        listenerCleanups.push({ cleanup, listener })
-      }
+      listener(currentValue)
     }
   }
 
@@ -76,15 +55,6 @@ export function createFacet<V>({
    */
   const updateToNoValue = () => {
     currentValue = NO_VALUE
-
-    if (listenerCleanups.length !== 0) {
-      for (let index = 0; index < listenerCleanups.length; index++) {
-        listenerCleanups[index].cleanup()
-      }
-
-      // start with a new array
-      listenerCleanups = []
-    }
   }
 
   return {
@@ -105,12 +75,7 @@ export function createFacet<V>({
       listeners.add(listener)
 
       if (currentValue !== NO_VALUE) {
-        const cleanup = listener(currentValue)
-
-        // if the listener returns a cleanup function, we store it to call latter
-        if (cleanup != null) {
-          listenerCleanups.push({ cleanup, listener })
-        }
+        listener(currentValue)
       }
 
       // This is the first subscription, so we start subscribing to dependencies
@@ -119,13 +84,6 @@ export function createFacet<V>({
       }
 
       return () => {
-        // check if this listener has any cleanup that we need to call
-        const cleanupIndex = listenerCleanups.findIndex((entry) => entry.listener === listener)
-        if (cleanupIndex !== -1) {
-          listenerCleanups[cleanupIndex].cleanup()
-          listenerCleanups.splice(cleanupIndex, 1)
-        }
-
         listeners.delete(listener)
 
         // if this was the last to unsubscribe, we unsubscribe from our dependencies

packages/@react-facet/core/src/hooks/useFacetEffect.spec.tsx

@@ -10,13 +10,7 @@ it('triggers the effect on mount with the initial value and on any update of the
   const callback = jest.fn()
 
   const ComponentWithFacetEffect = () => {
-    useFacetEffect(
-      (value) => {
-        callback(value)
-      },
-      [],
-      [demoFacet],
-    )
+    useFacetEffect(callback, [], [demoFacet])
 
     return null
   }
@@ -75,15 +69,12 @@ describe('cleanup', () => {
     const demoFacet = createFacet({ initialValue: 'initial value' })
 
     const cleanup = jest.fn()
+    const effect = jest.fn((...args) => {
+      return () => cleanup(...args)
+    })
 
     const ComponentWithFacetEffect = () => {
-      useFacetEffect(
-        () => {
-          return cleanup
-        },
-        [],
-        [demoFacet],
-      )
+      useFacetEffect(effect, [], [demoFacet])
 
       return null
     }
@@ -95,21 +86,36 @@ describe('cleanup', () => {
     // cleanup is not called immediately
     expect(cleanup).not.toHaveBeenCalled()
 
+    // ...but the effect is
+    expect(effect).toHaveBeenCalledTimes(1)
+    expect(effect).toHaveBeenCalledWith('initial value')
+
+    effect.mockClear()
+
     act(() => {
       demoFacet.set('new value')
     })
 
     // once a new value is triggered we expect that the previous cleanup was called
-    expect(cleanup).toHaveBeenCalled()
+    expect(cleanup).toHaveBeenCalledTimes(1)
+    expect(cleanup).toHaveBeenCalledWith('initial value')
+
+    // ...and the effect is called again with the new value
+    expect(effect).toHaveBeenCalledTimes(1)
+    expect(effect).toHaveBeenLastCalledWith('new value')
 
-    // clear any recorded calls ahead of next check
     cleanup.mockClear()
+    effect.mockClear()
 
     // unmount the component to check if the cleanup is also called
     rerender(<></>)
 
-    // once a new value is triggered we expect that the previous cleanup was called
-    expect(cleanup).toHaveBeenCalled()
+    // when the component unmounts we expect that we cleanup the last called effect
+    expect(cleanup).toHaveBeenCalledTimes(1)
+    expect(cleanup).toHaveBeenCalledWith('new value')
+
+    // the effect shouldn't be called again on unmount
+    expect(effect).not.toHaveBeenCalled()
   })
 })
 
@@ -121,13 +127,7 @@ it('supports multiple facets, only triggering the effect once all facets have a
   const effect = jest.fn().mockReturnValue(cleanup)
 
   const ComponentWithFacetEffect = () => {
-    useFacetEffect(
-      (valueA, valueB) => {
-        return effect(valueA, valueB)
-      },
-      [],
-      [facetA, facetB],
-    )
+    useFacetEffect(effect, [], [facetA, facetB])
 
     return null
   }

packages/@react-facet/core/src/hooks/useFacetEffect.tsx

@@ -1,57 +1,75 @@
-import { useCallback, useEffect } from 'react'
+import { useCallback, useEffect, useLayoutEffect } from 'react'
 import { Facet, Unsubscribe, Cleanup, NO_VALUE, ExtractFacetValues } from '../types'
 
-/**
- * Allows running an effect based on facet updates. Similar to React's useEffect.
- *
- * @param effect function that will do the side-effect (ex: update the DOM)
- * @param dependencies variable used by the map that are available in scope (similar as dependencies of useEffect)
- * @param facets facets that the effect listens to
- *
- * We pass the dependencies of the callback as the second argument so we can leverage the eslint-plugin-react-hooks option for additionalHooks.
- * Having this as the second argument allows the linter to work.
- */
-export function useFacetEffect<Y extends Facet<unknown>[], T extends [...Y]>(
-  effect: (...args: ExtractFacetValues<T>) => void | Cleanup,
-  dependencies: unknown[],
-  facets: T,
-) {
-  // eslint-disable-next-line react-hooks/exhaustive-deps
-  const effectMemoized = useCallback(effect as (...args: unknown[]) => ReturnType<typeof effect>, dependencies)
-
-  useEffect(() => {
-    if (facets.length === 1) {
-      return facets[0].observe(effectMemoized)
-    }
-
-    let cleanup: void | Cleanup
-    let hasAllDependencies = false
-    const unsubscribes: Unsubscribe[] = []
-    const values: unknown[] = facets.map(() => NO_VALUE)
-
-    facets.forEach((facet, index) => {
-      unsubscribes[index] = facet.observe((value) => {
-        values[index] = value
-        hasAllDependencies = hasAllDependencies || values.every((value) => value != NO_VALUE)
-
-        if (hasAllDependencies) {
+export const createUseFacetEffect = (useHook: typeof useEffect | typeof useLayoutEffect) => {
+  return function <Y extends Facet<unknown>[], T extends [...Y]>(
+    effect: (...args: ExtractFacetValues<T>) => void | Cleanup,
+    dependencies: unknown[],
+    facets: T,
+  ) {
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    const effectMemoized = useCallback(effect as (...args: unknown[]) => ReturnType<typeof effect>, dependencies)
+
+    useHook(() => {
+      let cleanup: void | Cleanup
+
+      if (facets.length === 1) {
+        const unsubscribe = facets[0].observe((value) => {
           if (cleanup != null) {
             cleanup()
           }
 
-          cleanup = effect(...(values as ExtractFacetValues<T>))
+          cleanup = effectMemoized(value)
+        })
+
+        return () => {
+          unsubscribe()
+          if (cleanup != null) {
+            cleanup()
+          }
         }
+      }
+
+      let hasAllDependencies = false
+      const unsubscribes: Unsubscribe[] = []
+      const values: unknown[] = facets.map(() => NO_VALUE)
+
+      facets.forEach((facet, index) => {
+        unsubscribes[index] = facet.observe((value) => {
+          values[index] = value
+          hasAllDependencies = hasAllDependencies || values.every((value) => value != NO_VALUE)
+
+          if (hasAllDependencies) {
+            if (cleanup != null) {
+              cleanup()
+            }
+
+            cleanup = effectMemoized(...(values as ExtractFacetValues<T>))
+          }
+        })
       })
-    })
 
-    return () => {
-      unsubscribes.forEach((unsubscribe) => unsubscribe())
-      if (cleanup != null) {
-        cleanup()
+      return () => {
+        unsubscribes.forEach((unsubscribe) => unsubscribe())
+        if (cleanup != null) {
+          cleanup()
+        }
       }
-    }
 
-    // We care about each individual facet and if any is a different reference
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [effectMemoized, ...facets])
+      // We care about each individual facet and if any is a different reference
+      // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, [effectMemoized, ...facets])
+  }
 }
+
+/**
+ * Allows running an effect based on facet updates. Similar to React's useEffect.
+ *
+ * @param effect function that will do the side-effect (ex: update the DOM)
+ * @param dependencies variable used by the map that are available in scope (similar as dependencies of useEffect)
+ * @param facets facets that the effect listens to
+ *
+ * We pass the dependencies of the callback as the second argument so we can leverage the eslint-plugin-react-hooks option for additionalHooks.
+ * Having this as the second argument allows the linter to work.
+ */
+export const useFacetEffect = createUseFacetEffect(useEffect)