From 82c64e1a49239158c0daa7f0d603d2ad2ee667a9 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Fri, 20 May 2022 19:10:43 +0200 Subject: [PATCH] Match Preact behavior for boolean props on custom elements (#24541) * Log unexpected warnings when testing with ReactDOMServerIntegrationTestUtils * Add test Following https://github.com/facebook/react/issues/9230#issuecomment-322007671 except that `foo={true}` renders an empty string. See https://github.com/facebook/react/issues/9230#issuecomment-1123464720 for rationale. * Match Preact behavior for boolean props on custom elements * Poke CircleCI --- .../__tests__/DOMPropertyOperations-test.js | 28 +++++++++++++++++-- .../src/__tests__/ReactDOMComponent-test.js | 8 ++++-- ...eactDOMServerIntegrationAttributes-test.js | 12 ++++++-- .../ReactDOMServerIntegrationTestUtils.js | 2 +- .../src/client/DOMPropertyOperations.js | 13 +++++++++ .../react-dom/src/client/ReactDOMComponent.js | 14 ++++++++-- .../src/server/ReactDOMServerFormatConfig.js | 8 +++++- packages/react-dom/src/shared/DOMProperty.js | 5 ++++ 8 files changed, 79 insertions(+), 11 deletions(-) diff --git a/packages/react-dom/src/__tests__/DOMPropertyOperations-test.js b/packages/react-dom/src/__tests__/DOMPropertyOperations-test.js index ab5ca9a549521..73e3b4b49bfcc 100644 --- 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(, container); + const customElement = container.querySelector('my-custom-element'); + + expect(customElement.getAttribute('foo')).toBe(''); + + // true => false + ReactDOM.render(, 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 index fb7d7cf41ada3..ec547d8cbaa83 100644 --- 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(, container); const node = container.firstChild; - expect(node.getAttribute('foo')).toBe('true'); + expect(node.getAttribute('foo')).toBe( + ReactFeatureFlags.enableCustomElementPropertySupport ? '' : 'true', + ); ReactDOM.render(, container); - expect(node.getAttribute('foo')).toBe('false'); + expect(node.getAttribute('foo')).toBe( + ReactFeatureFlags.enableCustomElementPropertySupport ? null : 'false', + ); ReactDOM.render(, container); expect(node.hasAttribute('foo')).toBe(false); ReactDOM.render(, container); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js index b32146d55e44e..13b57e08a282d 100644 --- 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(); - 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(); - 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 index f03825c16d442..5465d1f33d36f 100644 --- 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 index f3d6fdac89795..eed5767e8a002 100644 --- 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 index 75e017a451836..19eb70e583b39 100644 --- 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 index 4814bd0aa3334..c8d6d35ceb6be 100644 --- 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 index 278a9fd12b006..b5540debb71df 100644 --- 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) {