Match Preact behavior for boolean props on custom elements (#24541)

* Log unexpected warnings when testing with ReactDOMServerIntegrationTestUtils * Add test Following #9230 (comment) except that `foo={true}` renders an empty string. See #9230 (comment) for rationale. * Match Preact behavior for boolean props on custom elements * Poke CircleCI
facebook · May 20, 2022 · 82c64e1 · 82c64e1
1 parent 6e2f38f
commit 82c64e1
Show file tree

Hide file tree

Showing 8 changed files with 79 additions and 11 deletions.
diff --git a/packages/react-dom/src/__tests__/DOMPropertyOperations-test.js b/packages/react-dom/src/__tests__/DOMPropertyOperations-test.js
@@ -10,7 +10,10 @@
 'use strict';
 
 // Set by `yarn test-fire`.
-const {disableInputAttributeSyncing} = require('shared/ReactFeatureFlags');
+const {
+  enableCustomElementPropertySupport,
+  disableInputAttributeSyncing,
+} = require('shared/ReactFeatureFlags');
 
 describe('DOMPropertyOperations', () => {
   let React;
@@ -256,8 +259,12 @@ describe('DOMPropertyOperations', () => {
       expect(customElement.getAttribute('onstring')).toBe('hello');
       expect(customElement.getAttribute('onobj')).toBe('[object Object]');
       expect(customElement.getAttribute('onarray')).toBe('one,two');
-      expect(customElement.getAttribute('ontrue')).toBe('true');
-      expect(customElement.getAttribute('onfalse')).toBe('false');
+      expect(customElement.getAttribute('ontrue')).toBe(
+        enableCustomElementPropertySupport ? '' : 'true',
+      );
+      expect(customElement.getAttribute('onfalse')).toBe(
+        enableCustomElementPropertySupport ? null : 'false',
+      );
 
       // Dispatch the corresponding event names to make sure that nothing crashes.
       customElement.dispatchEvent(new Event('string'));
@@ -959,6 +966,21 @@ describe('DOMPropertyOperations', () => {
       expect(customElement.foo).toBe(null);
     });
 
+    // @gate enableCustomElementPropertySupport
+    it('boolean props should not be stringified in attributes', () => {
+      const container = document.createElement('div');
+      document.body.appendChild(container);
+      ReactDOM.render(<my-custom-element foo={true} />, container);
+      const customElement = container.querySelector('my-custom-element');
+
+      expect(customElement.getAttribute('foo')).toBe('');
+
+      // true => false
+      ReactDOM.render(<my-custom-element foo={false} />, container);
+
+      expect(customElement.getAttribute('foo')).toBe(null);
+    });
+
     // @gate enableCustomElementPropertySupport
     it('custom element custom event handlers assign multiple types', () => {
       const container = document.createElement('div');

diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
@@ -2689,9 +2689,13 @@ describe('ReactDOMComponent', () => {
       const container = document.createElement('div');
       ReactDOM.render(<some-custom-element foo={true} />, container);
       const node = container.firstChild;
-      expect(node.getAttribute('foo')).toBe('true');
+      expect(node.getAttribute('foo')).toBe(
+        ReactFeatureFlags.enableCustomElementPropertySupport ? '' : 'true',
+      );
       ReactDOM.render(<some-custom-element foo={false} />, container);
-      expect(node.getAttribute('foo')).toBe('false');
+      expect(node.getAttribute('foo')).toBe(
+        ReactFeatureFlags.enableCustomElementPropertySupport ? null : 'false',
+      );
       ReactDOM.render(<some-custom-element />, container);
       expect(node.hasAttribute('foo')).toBe(false);
       ReactDOM.render(<some-custom-element foo={true} />, container);

diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js
@@ -696,12 +696,20 @@ describe('ReactDOMServerIntegration', () => {
 
     itRenders('unknown boolean `true` attributes as strings', async render => {
       const e = await render(<custom-element foo={true} />);
-      expect(e.getAttribute('foo')).toBe('true');
+      if (ReactFeatureFlags.enableCustomElementPropertySupport) {
+        expect(e.getAttribute('foo')).toBe('');
+      } else {
+        expect(e.getAttribute('foo')).toBe('true');
+      }
     });
 
     itRenders('unknown boolean `false` attributes as strings', async render => {
       const e = await render(<custom-element foo={false} />);
-      expect(e.getAttribute('foo')).toBe('false');
+      if (ReactFeatureFlags.enableCustomElementPropertySupport) {
+        expect(e.getAttribute('foo')).toBe(null);
+      } else {
+        expect(e.getAttribute('foo')).toBe('false');
+      }
     });
 
     itRenders(

diff --git a/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js b/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js
@@ -89,7 +89,7 @@ module.exports = function(initModules) {
         console.log(
           `We expected ${count} warning(s), but saw ${filteredWarnings.length} warning(s).`,
         );
-        if (filteredWarnings.count > 0) {
+        if (filteredWarnings.length > 0) {
           console.log(`We saw these warnings:`);
           for (let i = 0; i < filteredWarnings.length; i++) {
             console.log(...filteredWarnings[i]);

diff --git a/packages/react-dom/src/client/DOMPropertyOperations.js b/packages/react-dom/src/client/DOMPropertyOperations.js
@@ -115,6 +115,7 @@ export function getValueForAttribute(
   node: Element,
   name: string,
   expected: mixed,
+  isCustomComponentTag: boolean,
 ): mixed {
   if (__DEV__) {
     if (!isAttributeNameSafe(name)) {
@@ -124,6 +125,13 @@ export function getValueForAttribute(
       return expected === undefined ? undefined : null;
     }
     const value = node.getAttribute(name);
+
+    if (enableCustomElementPropertySupport) {
+      if (isCustomComponentTag && value === '') {
+        return true;
+      }
+    }
+
     if (__DEV__) {
       checkAttributeStringCoercion(expected, name);
     }
@@ -196,6 +204,11 @@ export function setValueForProperty(
   if (shouldRemoveAttribute(name, value, propertyInfo, isCustomComponentTag)) {
     value = null;
   }
+  if (enableCustomElementPropertySupport) {
+    if (isCustomComponentTag && value === true) {
+      value = '';
+    }
+  }
 
   // If the prop isn't in the special list, treat it as a simple attribute.
   if (isCustomComponentTag || propertyInfo === null) {

diff --git a/packages/react-dom/src/client/ReactDOMComponent.js b/packages/react-dom/src/client/ReactDOMComponent.js
@@ -1081,7 +1081,12 @@ export function diffHydratedProperties(
       } else if (isCustomComponentTag && !enableCustomElementPropertySupport) {
         // $FlowFixMe - Should be inferred as not undefined.
         extraAttributeNames.delete(propKey.toLowerCase());
-        serverValue = getValueForAttribute(domElement, propKey, nextProp);
+        serverValue = getValueForAttribute(
+          domElement,
+          propKey,
+          nextProp,
+          isCustomComponentTag,
+        );
 
         if (nextProp !== serverValue) {
           warnForPropDifference(propKey, serverValue, nextProp);
@@ -1128,7 +1133,12 @@ export function diffHydratedProperties(
             // $FlowFixMe - Should be inferred as not undefined.
             extraAttributeNames.delete(propKey);
           }
-          serverValue = getValueForAttribute(domElement, propKey, nextProp);
+          serverValue = getValueForAttribute(
+            domElement,
+            propKey,
+            nextProp,
+            isCustomComponentTag,
+          );
         }
 
         const dontWarnCustomElement =

diff --git a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js
@@ -1156,7 +1156,7 @@ function pushStartCustomElement(
   let innerHTML = null;
   for (let propKey in props) {
     if (hasOwnProperty.call(props, propKey)) {
-      const propValue = props[propKey];
+      let propValue = props[propKey];
       if (propValue == null) {
         continue;
       }
@@ -1169,6 +1169,12 @@ function pushStartCustomElement(
         // so skip it.
         continue;
       }
+      if (enableCustomElementPropertySupport && propValue === false) {
+        continue;
+      }
+      if (enableCustomElementPropertySupport && propValue === true) {
+        propValue = '';
+      }
       if (enableCustomElementPropertySupport && propKey === 'className') {
         // className gets rendered as class on the client, so it should be
         // rendered as class on the server.

diff --git a/packages/react-dom/src/shared/DOMProperty.js b/packages/react-dom/src/shared/DOMProperty.js
@@ -162,6 +162,11 @@ export function shouldRemoveAttribute(
     return true;
   }
   if (isCustomComponentTag) {
+    if (enableCustomElementPropertySupport) {
+      if (value === false) {
+        return true;
+      }
+    }
     return false;
   }
   if (propertyInfo !== null) {