Fix unmount cleanup of Facets (#12)

The implementation in `@react-facet/dom-fiber` wasn't correctly removing all subscriptions of Facets when unmounting. 

This wasn't a problem in our original implementation of `@react-facet/dom-components`, and wasn't apparent in our current production screens (as they don't unmount), but can be quite problematic.

Added some extra unit tests.

Co-authored-by: Fernando Via Canel <fernando.via@gmail.com>

packages/@react-facet/dom-fiber/src/createFiberRoot.ts

@@ -8,6 +8,7 @@ export const createFiberRoot =
   (container: HTMLElement): FacetFiberRoot =>
     reconciler.createContainer(
       {
+        children: new Set(),
         element: container,
         styleUnsubscribers: new Map(),
         style: container.style,

packages/@react-facet/dom-fiber/src/createPortal.ts

@@ -14,8 +14,8 @@ export function createPortal(children: ReactNodeList, container: HTMLElement, ke
     key,
     children,
     containerInfo: {
+      children: new Set(),
       element: container,
-      unsubscribers: new Map(),
       styleUnsubscribers: new Map(),
       style: container.style,
     },

packages/@react-facet/dom-fiber/src/setupHostConfig.spec.tsx

@@ -901,10 +901,111 @@ describe('update', () => {
   })
 })
 
+describe('umnount', () => {
+  it('unsubscribes from all facets when a element component is unmounted', () => {
+    const unsubscribe = jest.fn()
+
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    const facet: Facet<any> = {
+      get: () => 'text',
+      observe: jest.fn().mockReturnValue(unsubscribe),
+    }
+
+    render(
+      <fast-div
+        style={{ background: facet, color: facet }}
+        className={facet}
+        data-droppable={facet}
+        data-testid={facet}
+        data-x-ray={facet}
+        src={facet}
+        href={facet}
+        target={facet}
+        autoPlay={facet}
+        loop={facet}
+        disabled={facet}
+        maxLength={facet}
+        rows={facet}
+        value={facet}
+        type={facet}
+      />,
+    )
+    // on mount, we verify that we have added 16 subscriptions (one for each prop and style above)
+    expect(facet.observe).toHaveBeenCalledTimes(16)
+
+    // on unmount, we check that unsubscribe was called once for each subscription
+    render(<></>)
+    expect(unsubscribe).toHaveBeenCalledTimes(16)
+  })
+
+  it('unsubscribes from the text facet when a fast-text component is unmounted', () => {
+    const unsubscribe = jest.fn()
+
+    const facet: Facet<string> = {
+      get: () => 'text',
+      observe: jest.fn().mockReturnValue(unsubscribe),
+    }
+
+    render(<fast-text text={facet} />)
+    expect(facet.observe).toHaveBeenCalledTimes(1)
+
+    render(<></>)
+    expect(unsubscribe).toHaveBeenCalledTimes(1)
+  })
+
+  it('unsubscribe from facets when the parent of a component is unmounted', () => {
+    const unsubscribe = jest.fn()
+
+    const facet: Facet<string> = {
+      get: () => 'text',
+      observe: jest.fn().mockReturnValue(unsubscribe),
+    }
+
+    render(
+      <div>
+        <fast-text text={facet} />
+      </div>,
+    )
+    expect(facet.observe).toHaveBeenCalledTimes(1)
+    expect(unsubscribe).toHaveBeenCalledTimes(0)
+
+    render(<></>)
+    expect(unsubscribe).toHaveBeenCalledTimes(1)
+  })
+
+  it('keeps the subscription of facets when moving in a keyed list', () => {
+    const unsubscribeA = jest.fn()
+
+    const facetA: Facet<string> = {
+      get: () => 'text',
+      observe: jest.fn().mockReturnValue(unsubscribeA),
+    }
+
+    const unsubscribeB = jest.fn()
+
+    const facetB: Facet<string> = {
+      get: () => 'text',
+      observe: jest.fn().mockReturnValue(unsubscribeB),
+    }
+
+    render(<div>{[<fast-div key={'A'} className={facetA} />, <fast-div key={'B'} className={facetB} />]}</div>)
+    expect(facetA.observe).toHaveBeenCalledTimes(1)
+    expect(facetB.observe).toHaveBeenCalledTimes(1)
+
+    render(<div>{[<fast-div key={'B'} className={facetB} />, <fast-div key={'A'} className={facetA} />]}</div>)
+
+    expect(facetA.observe).toHaveBeenCalledTimes(1)
+    expect(facetB.observe).toHaveBeenCalledTimes(1)
+    expect(unsubscribeA).not.toHaveBeenCalled()
+    expect(unsubscribeB).not.toHaveBeenCalled()
+  })
+})
+
 describe('commitUpdate style prop', () => {
   it('subscribes when updating from null', () => {
     const hostConfig = setupHostConfig()
     const instance: ElementContainer = {
+      children: new Set(),
       element: document.createElement('div'),
       styleUnsubscribers: new Map(),
     }
@@ -944,6 +1045,7 @@ describe('commitUpdate style prop', () => {
 
     const hostConfig = setupHostConfig()
     const instance: ElementContainer = {
+      children: new Set(),
       element: document.createElement('div'),
       styleUnsubscribers: new Map([['color', colorUnsubscriber]]),
     }
@@ -982,6 +1084,7 @@ describe('commitUpdate style prop', () => {
 
     const hostConfig = setupHostConfig()
     const instance: ElementContainer = {
+      children: new Set(),
       element: document.createElement('div'),
       styleUnsubscribers: new Map([['color', colorUnsubscriber]]),
     }
@@ -1015,6 +1118,7 @@ describe('commitUpdate style prop', () => {
 
     const hostConfig = setupHostConfig()
     const instance: ElementContainer = {
+      children: new Set(),
       element: document.createElement('div'),
       styleUnsubscribers: new Map([['color', colorUnsubscriber]]),
     }
@@ -1062,6 +1166,7 @@ describe('commitUpdate style prop', () => {
 
     const hostConfig = setupHostConfig()
     const instance: ElementContainer = {
+      children: new Set(),
       element: document.createElement('div'),
       styleUnsubscribers: new Map([['color', colorUnsubscriber]]),
     }

packages/@react-facet/dom-fiber/src/setupHostConfig.ts

@@ -2,13 +2,15 @@ import {
   Props,
   Type,
   Container,
+  ElementContainer,
   Instance,
   TextInstance,
   HydratableInstance,
   PublicInstance,
   HostContext,
   UpdatePayload,
   NoTimeout,
+  isElementContainer,
 } from './types'
 import { HostConfig } from 'react-reconciler'
 import { FacetProp, isFacet, Unsubscribe } from '@react-facet/core'
@@ -89,6 +91,7 @@ export const setupHostConfig = (): HostConfig<
       const element = document.createTextNode('')
 
       return {
+        children: new Set(),
         element,
         text: setupTextUpdate(newProps.text, element),
       }
@@ -188,6 +191,7 @@ export const setupHostConfig = (): HostConfig<
       element,
       styleUnsubscribers,
       style,
+      children: new Set(),
 
       className: newProps.className != null ? setupClassUpdate(newProps.className, element) : undefined,
       autoPlay: newProps.autoPlay != null ? setupAutoPlayUpdate(newProps.autoPlay, element) : undefined,
@@ -211,12 +215,6 @@ export const setupHostConfig = (): HostConfig<
     }
   },
 
-  appendInitialChild: function (parent, child) {
-    if (parent.element == null || child.element == null) return
-
-    parent.element.appendChild(child.element)
-  },
-
   finalizeInitialChildren: function () {
     return false
   },
@@ -227,12 +225,6 @@ export const setupHostConfig = (): HostConfig<
 
   commitMount: function () {},
 
-  appendChildToContainer: function (parent, child) {
-    if (parent.element == null || child.element == null) return
-
-    parent.element.appendChild(child.element)
-  },
-
   prepareUpdate: function () {
     return true
   },
@@ -531,7 +523,27 @@ export const setupHostConfig = (): HostConfig<
     textInstance.element.nodeValue = newText
   },
 
+  appendInitialChild: function (parent, child) {
+    if (isElementContainer(child)) {
+      parent.children.add(child)
+    }
+
+    parent.element.appendChild(child.element)
+  },
+
+  appendChildToContainer: function (parent, child) {
+    if (isElementContainer(child)) {
+      parent.children.add(child)
+    }
+
+    parent.element.appendChild(child.element)
+  },
+
   appendChild: function (parentInstance, child) {
+    if (isElementContainer(child)) {
+      parentInstance.children.add(child)
+    }
+
     parentInstance.element.appendChild(child.element)
   },
 
@@ -540,6 +552,10 @@ export const setupHostConfig = (): HostConfig<
   },
 
   removeChild: function (parentInstance, child) {
+    if (isElementContainer(child)) {
+      cleanupElementContainer(child)
+    }
+
     parentInstance.element.removeChild(child.element)
   },
 
@@ -548,6 +564,10 @@ export const setupHostConfig = (): HostConfig<
   },
 
   removeChildFromContainer: function (container, child) {
+    if (isElementContainer(child)) {
+      cleanupElementContainer(child)
+    }
+
     container.element.removeChild(child.element)
   },
 
@@ -564,6 +584,30 @@ export const setupHostConfig = (): HostConfig<
   },
 })
 
+const cleanupElementContainer = (instance: ElementContainer) => {
+  instance.styleUnsubscribers?.forEach((unsubscribe) => unsubscribe())
+  instance.styleUnsubscribers?.clear()
+
+  instance.children.forEach(cleanupElementContainer)
+  instance.children.clear()
+
+  instance.className?.()
+  instance['data-droppable']?.()
+  instance['data-testid']?.()
+  instance['data-x-ray']?.()
+  instance.src?.()
+  instance.href?.()
+  instance.target?.()
+  instance.autoPlay?.()
+  instance.loop?.()
+  instance.disabled?.()
+  instance.maxLength?.()
+  instance.rows?.()
+  instance.value?.()
+  instance.type?.()
+  instance.text?.()
+}
+
 const setupClassUpdate = (className: FacetProp<string | undefined>, element: HTMLElement) => {
   if (isFacet(className)) {
     return className.observe((className) => {

packages/@react-facet/dom-fiber/src/types.ts

@@ -116,6 +116,8 @@ export type Props<T> = ElementProps<T> & TextProps
 export type ValidPropsNames = keyof Props<unknown>
 
 export type ElementContainer = {
+  children: Set<ElementContainer>
+
   element: HTMLElement | Text
 
   style?: CSSStyleDeclaration
@@ -138,6 +140,10 @@ export type ElementContainer = {
   text?: Unsubscribe
 }
 
+export const isElementContainer = (value: ElementContainer | TextContainer): value is ElementContainer => {
+  return value != null && 'children' in value
+}
+
 export type TextContainer = {
   element: Text
 }