Filter certain DOM attributes (e.g. src, href) if their values are em…

…pty strings This prevents e.g. <img src=""> from making an unnecessar HTTP request for certain browsers.
facebook · Apr 6, 2020 · 06d07c0 · 06d07c0
1 parent c781156
commit 06d07c0
Show file tree

Hide file tree

Showing 12 changed files with 122 additions and 1 deletion.
diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
@@ -445,6 +445,60 @@ describe('ReactDOMComponent', () => {
       expect(node.hasAttribute('data-foo')).toBe(false);
     });
 
+    if (ReactFeatureFlags.enableAttributeEmptyStringFilter) {
+      it('should not add an empty src attribute', () => {
+        const container = document.createElement('div');
+        ReactDOM.render(<img src="" />, container);
+        const node = container.firstChild;
+        expect(node.hasAttribute('src')).toBe(false);
+
+        ReactDOM.render(<img src="abc" />, container);
+        expect(node.hasAttribute('src')).toBe(true);
+
+        ReactDOM.render(<img src="" />, container);
+        expect(node.hasAttribute('src')).toBe(false);
+      });
+
+      it('should not add an empty href attribute', () => {
+        const container = document.createElement('div');
+        ReactDOM.render(<link href="" />, container);
+        const node = container.firstChild;
+        expect(node.hasAttribute('href')).toBe(false);
+
+        ReactDOM.render(<link href="abc" />, container);
+        expect(node.hasAttribute('href')).toBe(true);
+
+        ReactDOM.render(<link href="" />, container);
+        expect(node.hasAttribute('href')).toBe(false);
+      });
+
+      it('should not add an empty action attribute', () => {
+        const container = document.createElement('div');
+        ReactDOM.render(<form action="" />, container);
+        const node = container.firstChild;
+        expect(node.hasAttribute('action')).toBe(false);
+
+        ReactDOM.render(<form action="abc" />, container);
+        expect(node.hasAttribute('action')).toBe(true);
+
+        ReactDOM.render(<form action="" />, container);
+        expect(node.hasAttribute('action')).toBe(false);
+      });
+
+      it('should not add an empty formAction attribute', () => {
+        const container = document.createElement('div');
+        ReactDOM.render(<button formAction="" />, container);
+        const node = container.firstChild;
+        expect(node.hasAttribute('formAction')).toBe(false);
+
+        ReactDOM.render(<button formAction="abc" />, container);
+        expect(node.hasAttribute('formAction')).toBe(true);
+
+        ReactDOM.render(<button formAction="" />, container);
+        expect(node.hasAttribute('formAction')).toBe(false);
+      });
+    }
+
     it('should apply React-specific aliases to HTML elements', () => {
       const container = document.createElement('div');
       ReactDOM.render(<form acceptCharset="foo" />, container);

diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js
@@ -10,6 +10,7 @@
 'use strict';
 
 const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils');
+const ReactFeatureFlags = require('shared/ReactFeatureFlags');
 
 let React;
 let ReactDOM;
@@ -331,6 +332,30 @@ describe('ReactDOMServerIntegration', () => {
       });
     });
 
+    if (ReactFeatureFlags.enableAttributeEmptyStringFilter) {
+      describe('special empty string property', function() {
+        itRenders('action prop', async render => {
+          const e = await render(<form action="" />);
+          expect(e.hasAttribute('action')).toBe(false);
+        });
+
+        itRenders('formAction prop', async render => {
+          const e = await render(<button formAction="" />);
+          expect(e.hasAttribute('formAction')).toBe(false);
+        });
+
+        itRenders('href prop', async render => {
+          const e = await render(<link href="" />);
+          expect(e.hasAttribute('href')).toBe(false);
+        });
+
+        itRenders('src prop', async render => {
+          const e = await render(<img src="" />);
+          expect(e.hasAttribute('src')).toBe(false);
+        });
+      });
+    }
+
     describe('props with special meaning in React', function() {
       itRenders('no ref attribute', async render => {
         class RefComponent extends React.Component {

diff --git a/packages/react-dom/src/shared/DOMProperty.js b/packages/react-dom/src/shared/DOMProperty.js
@@ -7,7 +7,10 @@
  * @flow
  */
 
-import {enableDeprecatedFlareAPI} from 'shared/ReactFeatureFlags';
+import {
+  enableDeprecatedFlareAPI,
+  enableFilterEmptyStringAttributesDOM,
+} from 'shared/ReactFeatureFlags';
 
 type PropertyType = 0 | 1 | 2 | 3 | 4 | 5 | 6;
 
@@ -52,6 +55,7 @@ export type PropertyInfo = {|
   +propertyName: string,
   +type: PropertyType,
   +sanitizeURL: boolean,
+  +removeEmptyString: boolean,
 |};
 
 /* eslint-disable max-len */
@@ -146,6 +150,15 @@ export function shouldRemoveAttribute(
   propertyInfo: PropertyInfo | null,
   isCustomComponentTag: boolean,
 ): boolean {
+  if (enableFilterEmptyStringAttributesDOM) {
+    if (
+      propertyInfo !== null &&
+      propertyInfo.removeEmptyString === true &&
+      value === ''
+    ) {
+      return true;
+    }
+  }
   if (value === null || typeof value === 'undefined') {
     return true;
   }
@@ -188,6 +201,7 @@ function PropertyInfoRecord(
   attributeName: string,
   attributeNamespace: string | null,
   sanitizeURL: boolean,
+  removeEmptyString: boolean,
 ) {
   this.acceptsBooleans =
     type === BOOLEANISH_STRING ||
@@ -199,6 +213,7 @@ function PropertyInfoRecord(
   this.propertyName = name;
   this.type = type;
   this.sanitizeURL = sanitizeURL;
+  this.removeEmptyString = removeEmptyString;
 }
 
 // When adding attributes to this list, be sure to also add them to
@@ -232,6 +247,7 @@ reservedProps.forEach(name => {
     name, // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -250,6 +266,7 @@ reservedProps.forEach(name => {
     attributeName, // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -264,6 +281,7 @@ reservedProps.forEach(name => {
     name.toLowerCase(), // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -284,6 +302,7 @@ reservedProps.forEach(name => {
     name, // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -322,6 +341,7 @@ reservedProps.forEach(name => {
     name.toLowerCase(), // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -346,6 +366,7 @@ reservedProps.forEach(name => {
     name, // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -366,6 +387,7 @@ reservedProps.forEach(name => {
     name, // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -387,6 +409,7 @@ reservedProps.forEach(name => {
     name, // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -399,6 +422,7 @@ reservedProps.forEach(name => {
     name.toLowerCase(), // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -497,6 +521,7 @@ const capitalize = token => token[1].toUpperCase();
     attributeName,
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -521,6 +546,7 @@ const capitalize = token => token[1].toUpperCase();
     attributeName,
     'http://www.w3.org/1999/xlink',
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -542,6 +568,7 @@ const capitalize = token => token[1].toUpperCase();
     attributeName,
     'http://www.w3.org/XML/1998/namespace',
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -556,6 +583,7 @@ const capitalize = token => token[1].toUpperCase();
     attributeName.toLowerCase(), // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -569,6 +597,7 @@ properties[xlinkHref] = new PropertyInfoRecord(
   'xlink:href',
   'http://www.w3.org/1999/xlink',
   true, // sanitizeURL
+  false, // removeEmptyString
 );
 
 ['src', 'href', 'action', 'formAction'].forEach(attributeName => {
@@ -579,5 +608,6 @@ properties[xlinkHref] = new PropertyInfoRecord(
     attributeName.toLowerCase(), // attributeName
     null, // attributeNamespace
     true, // sanitizeURL
+    true, // removeEmptyString
   );
 });
diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js
@@ -7,6 +7,10 @@
  * @flow strict
  */
 
+// Filter certain DOM attributes (e.g. src, href) if their values are empty strings.
+// This prevents e.g. <img src=""> from making an unnecessar HTTP request for certain browsers.
+export const enableFilterEmptyStringAttributesDOM = false;
+
 // Helps identify side effects in render-phase lifecycle hooks and setState
 // reducers by double invoking them in Strict Mode.
 export const debugRenderPhaseSideEffectsForStrictMode = __DEV__;

diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js
@@ -43,6 +43,7 @@ export const runAllPassiveEffectDestroysBeforeCreates = false;
 export const enableModernEventSystem = false;
 export const warnAboutSpreadingKeyToJSX = false;
 export const enableLegacyFBSupport = false;
+export const enableFilterEmptyStringAttributesDOM = false;
 
 // Internal-only attempt to debug a React Native issue. See D20130868.
 export const throwEarlyForMysteriousError = true;

diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js
@@ -42,6 +42,7 @@ export const runAllPassiveEffectDestroysBeforeCreates = false;
 export const enableModernEventSystem = false;
 export const warnAboutSpreadingKeyToJSX = false;
 export const enableLegacyFBSupport = false;
+export const enableFilterEmptyStringAttributesDOM = false;
 
 // Internal-only attempt to debug a React Native issue. See D20130868.
 export const throwEarlyForMysteriousError = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js
@@ -42,6 +42,7 @@ export const runAllPassiveEffectDestroysBeforeCreates = false;
 export const enableModernEventSystem = false;
 export const warnAboutSpreadingKeyToJSX = false;
 export const enableLegacyFBSupport = false;
+export const enableFilterEmptyStringAttributesDOM = false;
 
 // Internal-only attempt to debug a React Native issue. See D20130868.
 export const throwEarlyForMysteriousError = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
@@ -42,6 +42,7 @@ export const runAllPassiveEffectDestroysBeforeCreates = true;
 export const enableModernEventSystem = false;
 export const warnAboutSpreadingKeyToJSX = false;
 export const enableLegacyFBSupport = false;
+export const enableFilterEmptyStringAttributesDOM = false;
 
 // Internal-only attempt to debug a React Native issue. See D20130868.
 export const throwEarlyForMysteriousError = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js
@@ -42,6 +42,7 @@ export const runAllPassiveEffectDestroysBeforeCreates = false;
 export const enableModernEventSystem = false;
 export const warnAboutSpreadingKeyToJSX = false;
 export const enableLegacyFBSupport = false;
+export const enableFilterEmptyStringAttributesDOM = false;
 
 // Internal-only attempt to debug a React Native issue. See D20130868.
 export const throwEarlyForMysteriousError = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js
@@ -42,6 +42,7 @@ export const runAllPassiveEffectDestroysBeforeCreates = true;
 export const enableModernEventSystem = false;
 export const warnAboutSpreadingKeyToJSX = false;
 export const enableLegacyFBSupport = !__EXPERIMENTAL__;
+export const enableFilterEmptyStringAttributesDOM = false;
 
 // Internal-only attempt to debug a React Native issue. See D20130868.
 export const throwEarlyForMysteriousError = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js
@@ -16,6 +16,7 @@
 export const warnAboutSpreadingKeyToJSX = __VARIANT__;
 export const disableModulePatternComponents = __VARIANT__;
 export const disableInputAttributeSyncing = __VARIANT__;
+export const enableFilterEmptyStringAttributesDOM = __VARIANT__;
 
 // These are already tested in both modes using the build type dimension,
 // so we don't need to use __VARIANT__ to get extra coverage.

diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js
@@ -23,6 +23,7 @@ export const {
   warnAboutSpreadingKeyToJSX,
   replayFailedUnitOfWorkWithInvokeGuardedCallback,
   enableModernEventSystem,
+  enableFilterEmptyStringAttributesDOM,
 } = dynamicFeatureFlags;
 
 // On WWW, __EXPERIMENTAL__ is used for a new modern build.