[react] "Unset" missing properties (#4534)

.changeset/strange-peas-design.md

@@ -0,0 +1,13 @@
+---
+'@lit/react': patch
+---
+
+Wrapped components will now keep track of JSX props from previous render that were set as a property on the element, but are now missing, and set the property to `undefined`. Note, wrapped components still do not have "default props" and missing props will be treated the same as explicitly passing in `undefined`.
+
+This fixes the previously unexpected behavior where the following JSX when first rendered with a truthy condition
+
+```jsx
+return condition ? <WrappedInput disabled /> : <WrappedInput />;
+```
+
+would leave the `disabled` property and reflected attribute to be `true` even when the condition turns falsey.

package.json

@@ -171,7 +171,6 @@
         "./packages/labs/gen-wrapper-vue:test",
         "./packages/labs/nextjs:test",
         "./packages/labs/preact-signals:test",
-        "./packages/labs/react:test",
         "./packages/labs/rollup-plugin-minify-html-literals:test",
         "./packages/labs/ssr:test",
         "./packages/labs/ssr-dom-shim:test",

packages/react/src/create-component.ts

@@ -243,7 +243,7 @@ export const createComponent = <
   type Props = ComponentProps<I, E>;
 
   const ReactComponent = React.forwardRef<I, Props>((props, ref) => {
-    const prevPropsRef = React.useRef<Props | null>(null);
+    const prevElemPropsRef = React.useRef(new Map());
     const elementRef = React.useRef<I | null>(null);
 
     // Props to be passed to React.createElement
@@ -274,20 +274,26 @@ export const createComponent = <
         if (elementRef.current === null) {
           return;
         }
-        for (const prop in elementProps) {
+        const newElemProps = new Map();
+        for (const key in elementProps) {
           setProperty(
             elementRef.current,
-            prop,
-            props[prop],
-            prevPropsRef.current ? prevPropsRef.current[prop] : undefined,
+            key,
+            props[key],
+            prevElemPropsRef.current.get(key),
             events
           );
+          prevElemPropsRef.current.delete(key);
+          newElemProps.set(key, props[key]);
         }
-        // Note, the spirit of React might be to "unset" any old values that
-        // are no longer included; however, there's no reasonable value to set
-        // them to so we just leave the previous state as is.
-
-        prevPropsRef.current = props;
+        // "Unset" any props from previous render that no longer exist.
+        // Setting to `undefined` seems like the correct thing to "unset"
+        // but currently React will set it as `null`.
+        // See https://github.com/facebook/react/issues/28203
+        for (const [key, value] of prevElemPropsRef.current) {
+          setProperty(elementRef.current, key, undefined, value, events);
+        }
+        prevElemPropsRef.current = newElemProps;
       });
 
       // Empty dependency array so this will only run once after first render.

packages/react/src/test/create-component_test.tsx

@@ -334,10 +334,7 @@ suite('createComponent', () => {
       assert.equal(el.getAttribute('id'), 'foo');
       assert.equal(el.id, 'foo');
 
-      // We currently require passing `undefined` to "unset" rather than
-      // omitting the prop
-      // See https://github.com/lit/lit/issues/4227
-      render(<Tag id={undefined} />);
+      render(<Tag />);
       assert.equal(el.getAttribute('id'), null);
       assert.equal(el.id, '');
 
@@ -383,7 +380,7 @@ suite('createComponent', () => {
       assert.equal(el.getAttribute('hidden'), '');
       assert.equal(el.hidden, true);
 
-      render(<Tag hidden={undefined} />);
+      render(<Tag />);
       assert.equal(el.getAttribute('hidden'), null);
       assert.equal(el.hidden, false);
 
@@ -419,7 +416,7 @@ suite('createComponent', () => {
       assert.equal(el.getAttribute('draggable'), 'true');
       assert.equal(el.draggable, true);
 
-      render(<Tag draggable={undefined} />);
+      render(<Tag />);
       assert.equal(el.getAttribute('draggable'), null);
       assert.equal(el.draggable, false);
 
@@ -457,7 +454,7 @@ suite('createComponent', () => {
       render(<Tag aria-checked />);
       assert.equal(el.getAttribute('aria-checked'), 'true');
 
-      render(<Tag aria-checked={undefined} />);
+      render(<Tag />);
       assert.equal(el.getAttribute('aria-checked'), null);
     });
   }
@@ -486,21 +483,19 @@ suite('createComponent', () => {
     barEvent = undefined;
 
     // Clear listener
+    // Explicitly setting `undefined` or omitting prop will clear listeners
     render(<BasicElementComponent onFoo={undefined} />);
     el.fire('foo');
     assert.equal(fooEvent, undefined);
     el.fire('bar');
-    // We do not clear event listeners unless undefined is explicitly passed in
-    assert.equal(barEvent!.type, 'bar');
+    assert.equal(barEvent, undefined);
     fooEvent = undefined;
     barEvent = undefined;
 
     // Reattach listener
     render(<BasicElementComponent onFoo={onFoo} />);
     el.fire('foo');
     assert.equal(fooEvent!.type, 'foo');
-    el.fire('bar');
-    assert.equal(barEvent!.type, 'bar');
 
     // Replace listener
     render(<BasicElementComponent onFoo={onFoo2} />);