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 7, 2020 · f8ac921 · f8ac921
1 parent 3278d24
commit f8ac921
Show file tree

Hide file tree

Showing 11 changed files with 134 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,93 @@ describe('ReactDOMComponent', () => {
       expect(node.hasAttribute('data-foo')).toBe(false);
     });
 
+    if (ReactFeatureFlags.enableFilterEmptyStringAttributesDOM) {
+      it('should not add an empty src attribute', () => {
+        const container = document.createElement('div');
+        expect(() => ReactDOM.render(<img src="" />, container)).toErrorDev(
+          'Invalid value "" (empty string) for attribute `src`.',
+        );
+        const node = container.firstChild;
+        expect(node.hasAttribute('src')).toBe(false);
+
+        ReactDOM.render(<img src="abc" />, container);
+        expect(node.hasAttribute('src')).toBe(true);
+
+        expect(() => ReactDOM.render(<img src="" />, container)).toErrorDev(
+          'Invalid value "" (empty string) for attribute `src`.',
+        );
+        expect(node.hasAttribute('src')).toBe(false);
+      });
+
+      it('should not add an empty href attribute', () => {
+        const container = document.createElement('div');
+        expect(() => ReactDOM.render(<link href="" />, container)).toErrorDev(
+          'Invalid value "" (empty string) for attribute `href`.',
+        );
+        const node = container.firstChild;
+        expect(node.hasAttribute('href')).toBe(false);
+
+        ReactDOM.render(<link href="abc" />, container);
+        expect(node.hasAttribute('href')).toBe(true);
+
+        expect(() => ReactDOM.render(<link href="" />, container)).toErrorDev(
+          'Invalid value "" (empty string) for attribute `href`.',
+        );
+        expect(node.hasAttribute('href')).toBe(false);
+      });
+
+      it('should not add an empty action attribute', () => {
+        const container = document.createElement('div');
+        expect(() => ReactDOM.render(<form action="" />, container)).toErrorDev(
+          'Invalid value "" (empty string) for attribute `action`.',
+        );
+        const node = container.firstChild;
+        expect(node.hasAttribute('action')).toBe(false);
+
+        ReactDOM.render(<form action="abc" />, container);
+        expect(node.hasAttribute('action')).toBe(true);
+
+        expect(() => ReactDOM.render(<form action="" />, container)).toErrorDev(
+          'Invalid value "" (empty string) for attribute `action`.',
+        );
+        expect(node.hasAttribute('action')).toBe(false);
+      });
+
+      it('should not add an empty formAction attribute', () => {
+        const container = document.createElement('div');
+        expect(() =>
+          ReactDOM.render(<button formAction="" />, container),
+        ).toErrorDev(
+          'Invalid value "" (empty string) for attribute `formAction`.',
+        );
+        const node = container.firstChild;
+        expect(node.hasAttribute('formAction')).toBe(false);
+
+        ReactDOM.render(<button formAction="abc" />, container);
+        expect(node.hasAttribute('formAction')).toBe(true);
+
+        expect(() =>
+          ReactDOM.render(<button formAction="" />, container),
+        ).toErrorDev(
+          'Invalid value "" (empty string) for attribute `formAction`.',
+        );
+        expect(node.hasAttribute('formAction')).toBe(false);
+      });
+
+      it('should not filter attributes for custom elements', () => {
+        const container = document.createElement('div');
+        ReactDOM.render(
+          <some-custom-element action="" formAction="" href="" src="" />,
+          container,
+        );
+        const node = container.firstChild;
+        expect(node.hasAttribute('action')).toBe(true);
+        expect(node.hasAttribute('formAction')).toBe(true);
+        expect(node.hasAttribute('href')).toBe(true);
+        expect(node.hasAttribute('src')).toBe(true);
+      });
+    }
+
     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/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 */
@@ -163,6 +167,19 @@ export function shouldRemoveAttribute(
     return false;
   }
   if (propertyInfo !== null) {
+    if (enableFilterEmptyStringAttributesDOM) {
+      if (propertyInfo.removeEmptyString && value === '') {
+        if (__DEV__) {
+          console.error(
+            'Invalid value "" (empty string) for attribute `%s`. ' +
+              'Use `null` instead to indicate an empty value.',
+            name,
+          );
+        }
+        return true;
+      }
+    }
+
     switch (propertyInfo.type) {
       case BOOLEAN:
         return !value;
@@ -188,6 +205,7 @@ function PropertyInfoRecord(
   attributeName: string,
   attributeNamespace: string | null,
   sanitizeURL: boolean,
+  removeEmptyString: boolean,
 ) {
   this.acceptsBooleans =
     type === BOOLEANISH_STRING ||
@@ -199,6 +217,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 +251,7 @@ reservedProps.forEach(name => {
     name, // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -250,6 +270,7 @@ reservedProps.forEach(name => {
     attributeName, // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -264,6 +285,7 @@ reservedProps.forEach(name => {
     name.toLowerCase(), // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -284,6 +306,7 @@ reservedProps.forEach(name => {
     name, // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -322,6 +345,7 @@ reservedProps.forEach(name => {
     name.toLowerCase(), // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -346,6 +370,7 @@ reservedProps.forEach(name => {
     name, // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -366,6 +391,7 @@ reservedProps.forEach(name => {
     name, // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -387,6 +413,7 @@ reservedProps.forEach(name => {
     name, // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -399,6 +426,7 @@ reservedProps.forEach(name => {
     name.toLowerCase(), // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -497,6 +525,7 @@ const capitalize = token => token[1].toUpperCase();
     attributeName,
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -521,6 +550,7 @@ const capitalize = token => token[1].toUpperCase();
     attributeName,
     'http://www.w3.org/1999/xlink',
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -542,6 +572,7 @@ const capitalize = token => token[1].toUpperCase();
     attributeName,
     'http://www.w3.org/XML/1998/namespace',
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -556,6 +587,7 @@ const capitalize = token => token[1].toUpperCase();
     attributeName.toLowerCase(), // attributeName
     null, // attributeNamespace
     false, // sanitizeURL
+    false, // removeEmptyString
   );
 });
 
@@ -569,6 +601,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 +612,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
@@ -44,6 +44,7 @@ export const enableModernEventSystem = false;
 export const warnAboutSpreadingKeyToJSX = false;
 export const enableComponentStackLocations = 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
@@ -43,6 +43,7 @@ export const enableModernEventSystem = false;
 export const warnAboutSpreadingKeyToJSX = false;
 export const enableComponentStackLocations = 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
@@ -43,6 +43,7 @@ export const enableModernEventSystem = false;
 export const warnAboutSpreadingKeyToJSX = false;
 export const enableComponentStackLocations = 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
@@ -43,6 +43,7 @@ export const enableModernEventSystem = false;
 export const warnAboutSpreadingKeyToJSX = false;
 export const enableComponentStackLocations = 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
@@ -43,6 +43,7 @@ export const enableModernEventSystem = false;
 export const warnAboutSpreadingKeyToJSX = false;
 export const enableComponentStackLocations = 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
@@ -43,6 +43,7 @@ export const enableModernEventSystem = false;
 export const warnAboutSpreadingKeyToJSX = false;
 export const enableComponentStackLocations = 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
@@ -17,6 +17,7 @@ export const warnAboutSpreadingKeyToJSX = __VARIANT__;
 export const enableComponentStackLocations = __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
@@ -24,6 +24,7 @@ export const {
   enableComponentStackLocations,
   replayFailedUnitOfWorkWithInvokeGuardedCallback,
   enableModernEventSystem,
+  enableFilterEmptyStringAttributesDOM,
 } = dynamicFeatureFlags;
 
 // On WWW, __EXPERIMENTAL__ is used for a new modern build.