Fix Style updates not properly cleaning previous subscriptions (#11)

* Adds unit tests that checks that indeed we are adding more subscriptions to style Facets

* Adds new test that verifies we are subscribing when updating to a new facet

* Check if the value has changed before subscribing again

* Adds a couple more unit tests o updating styles

* Rename the tests

Author: pirelenito

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

@@ -1,8 +1,10 @@
-import { createFacet } from '@react-facet/core'
+import { createFacet, Facet } from '@react-facet/core'
 import React, { ReactElement, useEffect, useState } from 'react'
+import { Fiber } from 'react-reconciler'
 import { createFiberRoot } from './createFiberRoot'
 import { createReconciler } from './createReconciler'
-import { InputType } from './types'
+import { setupHostConfig } from './setupHostConfig'
+import { InputType, ElementContainer, ElementProps, Props } from './types'
 
 document.body.innerHTML = `<div id="root"></div>`
 
@@ -898,3 +900,202 @@ describe('update', () => {
     })
   })
 })
+
+describe('commitUpdate style prop', () => {
+  it('subscribes when updating from null', () => {
+    const hostConfig = setupHostConfig()
+    const instance: ElementContainer = {
+      element: document.createElement('div'),
+      styleUnsubscribers: new Map(),
+    }
+
+    const oldProps: ElementProps<HTMLDivElement> = {
+      style: {
+        color: undefined,
+      },
+    }
+
+    const newColorFacet: Facet<string> = {
+      get: () => 'blue',
+      observe: jest.fn(),
+    }
+
+    const newProps: ElementProps<HTMLDivElement> = {
+      style: {
+        color: newColorFacet,
+      },
+    }
+
+    hostConfig.commitUpdate?.(
+      instance,
+      true,
+      'fast-div',
+      oldProps as Props<HTMLDivElement>,
+      newProps as Props<HTMLDivElement>,
+      null as unknown as Fiber,
+    )
+
+    // Adds a new subscription to the new Facet
+    expect(newColorFacet.observe).toHaveBeenCalledTimes(1)
+  })
+
+  it('unsubscribes when updating to null', () => {
+    const colorUnsubscriber = jest.fn()
+
+    const hostConfig = setupHostConfig()
+    const instance: ElementContainer = {
+      element: document.createElement('div'),
+      styleUnsubscribers: new Map([['color', colorUnsubscriber]]),
+    }
+
+    const oldColorFacet: Facet<string> = {
+      get: () => 'blue',
+      observe: jest.fn(),
+    }
+
+    const oldProps: ElementProps<HTMLDivElement> = {
+      style: {
+        color: oldColorFacet,
+      },
+    }
+
+    const newProps: ElementProps<HTMLDivElement> = {
+      style: {
+        color: undefined,
+      },
+    }
+
+    hostConfig.commitUpdate?.(
+      instance,
+      true,
+      'fast-div',
+      oldProps as Props<HTMLDivElement>,
+      newProps as Props<HTMLDivElement>,
+      null as unknown as Fiber,
+    )
+
+    expect(colorUnsubscriber).toHaveBeenCalledTimes(1)
+  })
+
+  it('unsubscribes from previous facet when changing to a primitive value', () => {
+    const colorUnsubscriber = jest.fn()
+
+    const hostConfig = setupHostConfig()
+    const instance: ElementContainer = {
+      element: document.createElement('div'),
+      styleUnsubscribers: new Map([['color', colorUnsubscriber]]),
+    }
+
+    const oldProps: ElementProps<HTMLDivElement> = {
+      style: {
+        color: createFacet({ initialValue: 'blue' }),
+      },
+    }
+
+    const newProps: ElementProps<HTMLDivElement> = {
+      style: {
+        color: 'yellow',
+      },
+    }
+
+    hostConfig.commitUpdate?.(
+      instance,
+      true,
+      'fast-div',
+      oldProps as Props<HTMLDivElement>,
+      newProps as Props<HTMLDivElement>,
+      null as unknown as Fiber,
+    )
+
+    expect(colorUnsubscriber).toHaveBeenCalledTimes(1)
+  })
+
+  it('unsubscribes from previous facet when changing to a new facet', () => {
+    const colorUnsubscriber = jest.fn()
+
+    const hostConfig = setupHostConfig()
+    const instance: ElementContainer = {
+      element: document.createElement('div'),
+      styleUnsubscribers: new Map([['color', colorUnsubscriber]]),
+    }
+
+    const oldColorFacet: Facet<string> = {
+      get: () => 'blue',
+      observe: jest.fn(),
+    }
+
+    const oldProps: ElementProps<HTMLDivElement> = {
+      style: {
+        color: oldColorFacet,
+      },
+    }
+
+    const newColorFacet: Facet<string> = {
+      get: () => 'blue',
+      observe: jest.fn(),
+    }
+
+    const newProps: ElementProps<HTMLDivElement> = {
+      style: {
+        color: newColorFacet,
+      },
+    }
+
+    hostConfig.commitUpdate?.(
+      instance,
+      true,
+      'fast-div',
+      oldProps as Props<HTMLDivElement>,
+      newProps as Props<HTMLDivElement>,
+      null as unknown as Fiber,
+    )
+
+    // Unsubscribes from the old subscription, since it is a new Facet
+    expect(colorUnsubscriber).toHaveBeenCalledTimes(1)
+
+    // Adds a new subscription to the new Facet
+    expect(newColorFacet.observe).toHaveBeenCalledTimes(1)
+  })
+
+  it('keeps the same subscription when updating with the same facet', () => {
+    const colorUnsubscriber = jest.fn()
+
+    const hostConfig = setupHostConfig()
+    const instance: ElementContainer = {
+      element: document.createElement('div'),
+      styleUnsubscribers: new Map([['color', colorUnsubscriber]]),
+    }
+
+    const colorFacet: Facet<string> = {
+      get: () => 'blue',
+      observe: jest.fn(),
+    }
+
+    const oldProps: ElementProps<HTMLDivElement> = {
+      style: {
+        color: colorFacet,
+      },
+    }
+
+    const newProps: ElementProps<HTMLDivElement> = {
+      style: {
+        color: colorFacet,
+      },
+    }
+
+    hostConfig.commitUpdate?.(
+      instance,
+      true,
+      'fast-div',
+      oldProps as Props<HTMLDivElement>,
+      newProps as Props<HTMLDivElement>,
+      null as unknown as Fiber,
+    )
+
+    // I shouldn't unsubscribe, since it is the same Facet
+    expect(colorUnsubscriber).not.toHaveBeenCalled()
+
+    // So I must not also observe again, since I should stick with the previous subscription
+    expect(colorFacet.observe).toHaveBeenCalledTimes(0)
+  })
+})

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

@@ -279,17 +279,20 @@ export const setupHostConfig = (): HostConfig<
 
       if (newStyleProp != null) {
         for (const key in newStyleProp) {
-          const value = newProps.style?.[key]
-
-          if (isFacet(value)) {
-            styleUnsubscribers.set(
-              key,
-              value.observe((value) => {
-                notNullStyle[key] = value
-              }),
-            )
-          } else {
-            notNullStyle[key] = value
+          const oldValue = oldStyleProp?.[key]
+          const newValue = newStyleProp[key]
+
+          if (oldValue !== newValue || oldStyleProp == null) {
+            if (isFacet(newValue)) {
+              styleUnsubscribers.set(
+                key,
+                newValue.observe((value) => {
+                  notNullStyle[key] = value
+                }),
+              )
+            } else {
+              notNullStyle[key] = newValue
+            }
           }
         }
       }