fix(react): skip undefined props in attachProps (v9)

packages/react/src/components/__tests__/utils.spec.ts

@@ -52,6 +52,69 @@ describe('attachProps', () => {
   });
 });
 
+describe('attachProps nullish props', () => {
+  it('should not write undefined props to a dom node', () => {
+    const div = document.createElement('div');
+
+    utils.attachProps(div, { id: undefined, title: undefined, testprop: undefined });
+
+    expect(div.hasAttribute('id')).toBe(false);
+    expect(div.hasAttribute('title')).toBe(false);
+    expect('testprop' in div).toBe(false);
+  });
+
+  it('should not write null native props to a dom node', () => {
+    const div = document.createElement('div');
+
+    utils.attachProps(div, { id: null, title: null, slot: null });
+
+    expect(div.hasAttribute('id')).toBe(false);
+    expect(div.hasAttribute('title')).toBe(false);
+    expect(div.hasAttribute('slot')).toBe(false);
+  });
+
+  it('should clear a prop that no longer has a value', () => {
+    const div = document.createElement('div');
+    utils.attachProps(div, { id: 'my-id', testprop: ['red'] });
+
+    utils.attachProps(div, { id: undefined, testprop: undefined }, { id: 'my-id', testprop: ['red'] });
+
+    expect(div.hasAttribute('id')).toBe(false);
+    expect((div as any).testprop).toBe(undefined);
+  });
+
+  it('should clear a native prop set to null', () => {
+    const div = document.createElement('div');
+    utils.attachProps(div, { id: 'my-id' });
+
+    utils.attachProps(div, { id: null }, { id: 'my-id' });
+
+    expect(div.hasAttribute('id')).toBe(false);
+  });
+
+  it('should treat null as a value for a prop the element does not natively have', () => {
+    const div = document.createElement('div');
+    utils.attachProps(div, { value: 'my-value' });
+
+    utils.attachProps(div, { value: null }, { value: 'my-value' });
+
+    expect((div as any).value).toBe(null);
+  });
+
+  it('should clear both attribute spellings of a camel cased native prop', () => {
+    const div = document.createElement('div');
+    // The property write reflects to `accesskey` while the dash-cased write
+    // adds `access-key`, so both attributes end up on the element.
+    utils.attachProps(div, { accessKey: 'k', tabIndex: 2 });
+
+    utils.attachProps(div, { accessKey: undefined, tabIndex: undefined }, { accessKey: 'k', tabIndex: 2 });
+
+    expect(div.hasAttribute('accesskey')).toBe(false);
+    expect(div.hasAttribute('access-key')).toBe(false);
+    expect(div.hasAttribute('tabindex')).toBe(false);
+  });
+});
+
 describe('attachProps boolean attributes', () => {
   it('should strip a stray disabled="false" attribute when the prop is false', () => {
     const div = document.createElement('div');

packages/react/src/components/react-component-lib/__tests__/createComponent.spec.tsx

@@ -44,3 +44,56 @@ describe('createReactComponent boolean attributes', () => {
     expect(el.hasAttribute('disabled')).toBe(false);
   });
 });
+
+/**
+ * These only fail at the wrapper level: render() omits a nullish prop, so React
+ * emits no attribute, and componentDidUpdate then writes one back through
+ * attachProps. A direct attachProps call cannot see that interaction.
+ */
+describe('createReactComponent nullish props', () => {
+  it('should not render an attribute for a prop passed as undefined', () => {
+    const { container } = render(
+      <ReactEl id={undefined} title={undefined}>
+        x
+      </ReactEl>
+    );
+    const el = container.querySelector('fake-react-el')!;
+
+    expect(el.hasAttribute('id')).toBe(false);
+    expect(el.hasAttribute('title')).toBe(false);
+  });
+
+  it('should not render an attribute for a prop passed as null', () => {
+    const { container } = render(
+      <ReactEl id={null} title={null}>
+        x
+      </ReactEl>
+    );
+    const el = container.querySelector('fake-react-el')!;
+
+    expect(el.hasAttribute('id')).toBe(false);
+    expect(el.hasAttribute('title')).toBe(false);
+  });
+
+  it('should drop the attribute when a prop becomes undefined', () => {
+    const { container, rerender } = render(<ReactEl id="my-id">x</ReactEl>);
+    const el = container.querySelector('fake-react-el')!;
+
+    act(() => {
+      rerender(<ReactEl id={undefined}>x</ReactEl>);
+    });
+
+    expect(el.hasAttribute('id')).toBe(false);
+  });
+
+  it('should drop the attribute when a prop becomes null', () => {
+    const { container, rerender } = render(<ReactEl id="my-id">x</ReactEl>);
+    const el = container.querySelector('fake-react-el')!;
+
+    act(() => {
+      rerender(<ReactEl id={null}>x</ReactEl>);
+    });
+
+    expect(el.hasAttribute('id')).toBe(false);
+  });
+});

packages/react/src/components/react-component-lib/utils/attachProps.ts

@@ -25,6 +25,16 @@ const NON_BOOLEAN_FALSE_ATTRIBUTES = new Set(['draggable', 'translate', 'spell-c
 const isStaleFalseBooleanAttribute = (attribute: string) =>
   !attribute.startsWith('aria-') && !attribute.startsWith('data-') && !NON_BOOLEAN_FALSE_ATTRIBUTES.has(attribute);
 
+/**
+ * A prop that every element already has is a native property: it mirrors an
+ * attribute the element owns, and assigning to it stringifies the value, so
+ * `node.id = undefined` leaves `id="undefined"` and `node.tabIndex = undefined`
+ * leaves `tabindex="0"`. Anything else is a component prop, where `null` can be
+ * a real value (`ion-input` declares `value?: string | number | null`), so it
+ * must still be assigned.
+ */
+const isNativeElementProperty = (name: string) => name in HTMLElement.prototype;
+
 export const attachProps = (node: HTMLElement, newProps: any, oldProps: any = {}) => {
   // some test frameworks don't render DOM elements, so we test here to make sure we are dealing with DOM first
   if (node instanceof Element) {
@@ -53,11 +63,46 @@ export const attachProps = (node: HTMLElement, newProps: any, oldProps: any = {}
           syncEvent(node, eventNameLc, newProps[name]);
         }
       } else {
-        (node as any)[name] = newProps[name];
-        const propType = typeof newProps[name];
+        const value = newProps[name];
+        const isNativeProperty = isNativeElementProperty(name);
+        if (value === undefined || (value === null && isNativeProperty)) {
+          /**
+           * Reflected properties such as `id`, `title` and `slot` stringify
+           * whatever they are given, so `node.id = undefined` leaves the element
+           * with the literal attribute `id="undefined"`. Never assign an
+           * undefined value. `null` stringifies the same way, but only a native
+           * property is treated as empty here, since a component prop may take
+           * `null` as a value.
+           *
+           * A prop that had a value and no longer does is a removal. A native
+           * property is cleared by dropping its attributes rather than by
+           * assigning, which would only coerce again, and it can carry two: the
+           * one it reflects to (`accesskey`) and the dash-cased one `render()`
+           * emits (`access-key`). Any other prop resets the property, which
+           * covers props with no attribute to mirror, then drops the attribute
+           * the string branch left behind.
+           */
+          const oldValue = oldProps[name];
+          if (oldValue !== undefined && oldValue !== null) {
+            const dashCasedName = camelToDashCase(name);
+            if (isNativeProperty) {
+              const reflectedName = name.toLowerCase();
+              node.removeAttribute(reflectedName);
+              if (dashCasedName !== reflectedName) {
+                node.removeAttribute(dashCasedName);
+              }
+            } else {
+              (node as any)[name] = undefined;
+              node.removeAttribute(dashCasedName);
+            }
+          }
+          return;
+        }
+        (node as any)[name] = value;
+        const propType = typeof value;
         if (propType === 'string') {
-          node.setAttribute(camelToDashCase(name), newProps[name]);
-        } else if (newProps[name] === false) {
+          node.setAttribute(camelToDashCase(name), value);
+        } else if (value === false) {
           const attribute = camelToDashCase(name);
           if (isStaleFalseBooleanAttribute(attribute)) {
             node.removeAttribute(attribute);