Split markup generation from DOM property management (#10197)

* Replace SSR unit test with integration test

* Remove unit test that is already covered by integration suite

* Replace unit tests for boolean attrs with integration tests

* Replace unit test for aria attrs with integration test

* Replace unit tests for numeric 0 with integration tests

* Remove unit test covered by integration tests

* Replace unit test for injection with integration test

It still touches internals but it tests both renderers.

* Fork DOMPropertyOperations into DOMMarkupOperations

* Trim down DOMPropertyOperations and DOMMarkupOperations

* Record SSR sizes

* Record them tests

* Fix false positive warning for overloaded booleans when passing numbers

* Remove stray import

* Replace CSS markup tests with public API tests

Some of these are handy as integration tests so I moved them there.
But some test markup specifically so I changed them to use DOMServer.

* Make CSSPropertyOperations client-only

I forked createMarkupForStyles() into ReactDOMComponent and ReactPartialRenderer. Duplication is fine because one of them will soon be gone (guess which one!)

The warnInvalidStyle helper is used by both server and client, unlike other client-only stuff in CSSPropertyOperations, so I moved it to a separately module used in both.

* Record server bundle size

* Add an early exit to validation

* Clarify what is being duplicated

Author: gaearon

scripts/fiber/tests-passing.txt

@@ -65,20 +65,20 @@
       "gzip": 81957
     },
     "react-dom-server.development.js (UMD_DEV)": {
-      "size": 152929,
-      "gzip": 37451
+      "size": 119540,
+      "gzip": 30394
     },
     "react-dom-server.production.min.js (UMD_PROD)": {
-      "size": 25022,
-      "gzip": 9357
+      "size": 23004,
+      "gzip": 8753
     },
     "react-dom-server.development.js (NODE_DEV)": {
-      "size": 120074,
-      "gzip": 29764
+      "size": 88637,
+      "gzip": 22952
     },
     "react-dom-server.production.min.js (NODE_PROD)": {
-      "size": 22559,
-      "gzip": 8401
+      "size": 20744,
+      "gzip": 7879
     },
     "ReactDOMServerStream-dev.js (FB_DEV)": {
       "size": 264750,
@@ -181,20 +181,20 @@
       "gzip": 50920
     },
     "ReactDOMServer-dev.js (FB_DEV)": {
-      "size": 119528,
-      "gzip": 29719
+      "size": 88149,
+      "gzip": 22908
     },
     "ReactDOMServer-prod.js (FB_PROD)": {
-      "size": 60237,
-      "gzip": 16296
+      "size": 53879,
+      "gzip": 14872
     },
     "react-dom-node-stream.development.js (NODE_DEV)": {
-      "size": 121768,
-      "gzip": 30291
+      "size": 90331,
+      "gzip": 23443
     },
     "react-dom-node-stream.production.min.js (NODE_PROD)": {
-      "size": 23522,
-      "gzip": 8727
+      "size": 21682,
+      "gzip": 8219
     },
     "ReactDOMNodeStream-dev.js (FB_DEV)": {
       "size": 264918,

scripts/rollup/results.json

@@ -14,21 +14,13 @@
 var CSSProperty = require('CSSProperty');
 var ExecutionEnvironment = require('fbjs/lib/ExecutionEnvironment');
 
-var camelizeStyleName = require('fbjs/lib/camelizeStyleName');
 var dangerousStyleValue = require('dangerousStyleValue');
-var getComponentName = require('getComponentName');
 var hyphenateStyleName = require('fbjs/lib/hyphenateStyleName');
-var memoizeStringOnly = require('fbjs/lib/memoizeStringOnly');
-var warning = require('fbjs/lib/warning');
 
 if (__DEV__) {
-  var {getCurrentFiberOwnerName} = require('ReactDebugCurrentFiber');
+  var warnValidStyle = require('warnValidStyle');
 }
 
-var processStyleName = memoizeStringOnly(function(styleName) {
-  return hyphenateStyleName(styleName);
-});
-
 var hasShorthandPropertyBug = false;
 if (ExecutionEnvironment.canUseDOM) {
   var tempStyle = document.createElement('div').style;
@@ -40,190 +32,15 @@ if (ExecutionEnvironment.canUseDOM) {
   }
 }
 
-if (__DEV__) {
-  // 'msTransform' is correct, but the other prefixes should be capitalized
-  var badVendoredStyleNamePattern = /^(?:webkit|moz|o)[A-Z]/;
-
-  // style values shouldn't contain a semicolon
-  var badStyleValueWithSemicolonPattern = /;\s*$/;
-
-  var warnedStyleNames = {};
-  var warnedStyleValues = {};
-  var warnedForNaNValue = false;
-  var warnedForInfinityValue = false;
-
-  var warnHyphenatedStyleName = function(name, owner) {
-    if (warnedStyleNames.hasOwnProperty(name) && warnedStyleNames[name]) {
-      return;
-    }
-
-    warnedStyleNames[name] = true;
-    warning(
-      false,
-      'Unsupported style property %s. Did you mean %s?%s',
-      name,
-      camelizeStyleName(name),
-      checkRenderMessage(owner),
-    );
-  };
-
-  var warnBadVendoredStyleName = function(name, owner) {
-    if (warnedStyleNames.hasOwnProperty(name) && warnedStyleNames[name]) {
-      return;
-    }
-
-    warnedStyleNames[name] = true;
-    warning(
-      false,
-      'Unsupported vendor-prefixed style property %s. Did you mean %s?%s',
-      name,
-      name.charAt(0).toUpperCase() + name.slice(1),
-      checkRenderMessage(owner),
-    );
-  };
-
-  var warnStyleValueWithSemicolon = function(name, value, owner) {
-    if (warnedStyleValues.hasOwnProperty(value) && warnedStyleValues[value]) {
-      return;
-    }
-
-    warnedStyleValues[value] = true;
-    warning(
-      false,
-      "Style property values shouldn't contain a semicolon.%s " +
-        'Try "%s: %s" instead.',
-      checkRenderMessage(owner),
-      name,
-      value.replace(badStyleValueWithSemicolonPattern, ''),
-    );
-  };
-
-  var warnStyleValueIsNaN = function(name, value, owner) {
-    if (warnedForNaNValue) {
-      return;
-    }
-
-    warnedForNaNValue = true;
-    warning(
-      false,
-      '`NaN` is an invalid value for the `%s` css style property.%s',
-      name,
-      checkRenderMessage(owner),
-    );
-  };
-
-  var warnStyleValueIsInfinity = function(name, value, owner) {
-    if (warnedForInfinityValue) {
-      return;
-    }
-
-    warnedForInfinityValue = true;
-    warning(
-      false,
-      '`Infinity` is an invalid value for the `%s` css style property.%s',
-      name,
-      checkRenderMessage(owner),
-    );
-  };
-
-  var checkRenderMessage = function(owner) {
-    var ownerName;
-    if (owner != null) {
-      // Stack passes the owner manually all the way to CSSPropertyOperations.
-      ownerName = getComponentName(owner);
-    } else {
-      // Fiber doesn't pass it but uses ReactDebugCurrentFiber to track it.
-      // It is only enabled in development and tracks host components too.
-      ownerName = getCurrentFiberOwnerName();
-      // TODO: also report the stack.
-    }
-    if (ownerName) {
-      return '\n\nCheck the render method of `' + ownerName + '`.';
-    }
-    return '';
-  };
-
-  /**
-   * @param {string} name
-   * @param {*} value
-   * @param {ReactDOMComponent} component
-   */
-  var warnValidStyle = function(name, value, component) {
-    var owner;
-    if (component) {
-      owner = component._currentElement._owner;
-    }
-    if (name.indexOf('-') > -1) {
-      warnHyphenatedStyleName(name, owner);
-    } else if (badVendoredStyleNamePattern.test(name)) {
-      warnBadVendoredStyleName(name, owner);
-    } else if (badStyleValueWithSemicolonPattern.test(value)) {
-      warnStyleValueWithSemicolon(name, value, owner);
-    }
-
-    if (typeof value === 'number') {
-      if (isNaN(value)) {
-        warnStyleValueIsNaN(name, value, owner);
-      } else if (!isFinite(value)) {
-        warnStyleValueIsInfinity(name, value, owner);
-      }
-    }
-  };
-}
-
 /**
  * Operations for dealing with CSS properties.
  */
 var CSSPropertyOperations = {
-  /**
-   * Serializes a mapping of style properties for use as inline styles:
-   *
-   *   > createMarkupForStyles({width: '200px', height: 0})
-   *   "width:200px;height:0;"
-   *
-   * Undefined values are ignored so that declarative programming is easier.
-   * The result should be HTML-escaped before insertion into the DOM.
-   *
-   * @param {object} styles
-   * @param {ReactDOMComponent} component
-   * @return {?string}
-   */
-  createMarkupForStyles: function(styles, component) {
-    var serialized = '';
-    var delimiter = '';
-    for (var styleName in styles) {
-      if (!styles.hasOwnProperty(styleName)) {
-        continue;
-      }
-      var isCustomProperty = styleName.indexOf('--') === 0;
-      var styleValue = styles[styleName];
-      if (__DEV__) {
-        if (!isCustomProperty) {
-          warnValidStyle(styleName, styleValue, component);
-        }
-      }
-      if (styleValue != null) {
-        serialized += delimiter + processStyleName(styleName) + ':';
-        serialized += dangerousStyleValue(
-          styleName,
-          styleValue,
-          isCustomProperty,
-        );
-
-        delimiter = ';';
-      }
-    }
-    return serialized || null;
-  },
-
   /**
    * This creates a string that is expected to be equivalent to the style
    * attribute generated by server-side rendering. It by-passes warnings and
    * security checks so it's not safe to use this value for anything other than
-   * comparison. It is only used in DEV for SSR validation. This is duplicated
-   * from createMarkupForStyles because createMarkupForStyles is expected to
-   * move out of the client-side renderer and it would be nice to make a clean
-   * break.
+   * comparison. It is only used in DEV for SSR validation.
    */
   createDangerousStringForStyles: function(styles) {
     if (__DEV__) {

src/renderers/dom/shared/CSSPropertyOperations.js

@@ -0,0 +1,125 @@
+/**
+ * Copyright 2013-present, Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ *
+ * @providesModule DOMMarkupOperations
+ */
+
+'use strict';
+
+var DOMProperty = require('DOMProperty');
+
+var quoteAttributeValueForBrowser = require('quoteAttributeValueForBrowser');
+var warning = require('fbjs/lib/warning');
+
+// isAttributeNameSafe() is currently duplicated in DOMPropertyOperations.
+// TODO: Find a better place for this.
+var VALID_ATTRIBUTE_NAME_REGEX = new RegExp(
+  '^[' +
+    DOMProperty.ATTRIBUTE_NAME_START_CHAR +
+    '][' +
+    DOMProperty.ATTRIBUTE_NAME_CHAR +
+    ']*$',
+);
+var illegalAttributeNameCache = {};
+var validatedAttributeNameCache = {};
+function isAttributeNameSafe(attributeName) {
+  if (validatedAttributeNameCache.hasOwnProperty(attributeName)) {
+    return true;
+  }
+  if (illegalAttributeNameCache.hasOwnProperty(attributeName)) {
+    return false;
+  }
+  if (VALID_ATTRIBUTE_NAME_REGEX.test(attributeName)) {
+    validatedAttributeNameCache[attributeName] = true;
+    return true;
+  }
+  illegalAttributeNameCache[attributeName] = true;
+  warning(false, 'Invalid attribute name: `%s`', attributeName);
+  return false;
+}
+
+// shouldIgnoreValue() is currently duplicated in DOMPropertyOperations.
+// TODO: Find a better place for this.
+function shouldIgnoreValue(propertyInfo, value) {
+  return (
+    value == null ||
+    (propertyInfo.hasBooleanValue && !value) ||
+    (propertyInfo.hasNumericValue && isNaN(value)) ||
+    (propertyInfo.hasPositiveNumericValue && value < 1) ||
+    (propertyInfo.hasOverloadedBooleanValue && value === false)
+  );
+}
+
+/**
+ * Operations for dealing with DOM properties.
+ */
+var DOMMarkupOperations = {
+  /**
+   * Creates markup for the ID property.
+   *
+   * @param {string} id Unescaped ID.
+   * @return {string} Markup string.
+   */
+  createMarkupForID: function(id) {
+    return (
+      DOMProperty.ID_ATTRIBUTE_NAME + '=' + quoteAttributeValueForBrowser(id)
+    );
+  },
+
+  createMarkupForRoot: function() {
+    return DOMProperty.ROOT_ATTRIBUTE_NAME + '=""';
+  },
+
+  /**
+   * Creates markup for a property.
+   *
+   * @param {string} name
+   * @param {*} value
+   * @return {?string} Markup string, or null if the property was invalid.
+   */
+  createMarkupForProperty: function(name, value) {
+    var propertyInfo = DOMProperty.properties.hasOwnProperty(name)
+      ? DOMProperty.properties[name]
+      : null;
+    if (propertyInfo) {
+      if (shouldIgnoreValue(propertyInfo, value)) {
+        return '';
+      }
+      var attributeName = propertyInfo.attributeName;
+      if (
+        propertyInfo.hasBooleanValue ||
+        (propertyInfo.hasOverloadedBooleanValue && value === true)
+      ) {
+        return attributeName + '=""';
+      }
+      return attributeName + '=' + quoteAttributeValueForBrowser(value);
+    } else if (DOMProperty.isCustomAttribute(name)) {
+      if (value == null) {
+        return '';
+      }
+      return name + '=' + quoteAttributeValueForBrowser(value);
+    }
+    return null;
+  },
+
+  /**
+   * Creates markup for a custom property.
+   *
+   * @param {string} name
+   * @param {*} value
+   * @return {string} Markup string, or empty string if the property was invalid.
+   */
+  createMarkupForCustomAttribute: function(name, value) {
+    if (!isAttributeNameSafe(name) || value == null) {
+      return '';
+    }
+    return name + '=' + quoteAttributeValueForBrowser(value);
+  },
+};
+
+module.exports = DOMMarkupOperations;