From 9254ce27a820c64fef51aeb4d39bdeeba5cb116d Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 19 Jul 2017 14:06:53 +0100 Subject: [PATCH] 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 --- scripts/fiber/tests-passing.txt | 158 +++++++++++--- scripts/rollup/results.json | 32 +-- .../dom/shared/CSSPropertyOperations.js | 187 +--------------- .../dom/shared/DOMMarkupOperations.js | 125 +++++++++++ .../dom/shared/DOMPropertyOperations.js | 74 +------ .../__tests__/CSSPropertyOperations-test.js | 112 +++------- .../__tests__/DOMPropertyOperations-test.js | 163 -------------- .../ReactDOMServerIntegration-test.js | 202 +++++++++++++++++- src/renderers/dom/shared/warnValidStyle.js | 150 +++++++++++++ .../dom/stack/client/ReactDOMComponent.js | 50 ++++- .../shared/server/ReactPartialRenderer.js | 52 ++++- 11 files changed, 742 insertions(+), 563 deletions(-) create mode 100644 src/renderers/dom/shared/DOMMarkupOperations.js create mode 100644 src/renderers/dom/shared/warnValidStyle.js diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 611dcac391069..809da38734f8e 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -683,15 +683,10 @@ src/renderers/dom/shared/__tests__/CSSProperty-test.js * should generate browser prefixes for its `isUnitlessNumber` src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js -* should create markup for simple styles -* should ignore undefined styles -* should ignore null styles -* should return null for no styles * should automatically append `px` to relevant styles * should trim values * should not append `px` to styles that might need a number * should create vendor-prefixed markup correctly -* should create markup with unitless css custom property * should set style attribute when styles exist * should not set style attribute when no styles exist * should warn when using hyphenated style names @@ -704,13 +699,6 @@ src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js * should not add units to CSS custom properties src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js -* should create markup for simple properties -* should work with the id attribute -* should create markup for boolean properties -* should create markup for booleanish properties -* should create markup for custom attributes -* should create markup for numeric properties -* should allow custom properties on web components * should set values as properties by default * should set values as attributes if necessary * should set values as namespace attributes if necessary @@ -728,7 +716,6 @@ src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js * should remove attributes for normal properties * should not remove attributes for special properties * should not leave all options selected when deleting multiple -* should support custom attributes src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js * should store a listener correctly @@ -895,11 +882,6 @@ src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js * renders a blank div with clean client render * renders a blank div with client render on top of good server markup * renders a blank div with client render on top of bad server markup -* renders a div with inline styles with server string render -* renders a div with inline styles with server stream render -* renders a div with inline styles with clean client render -* renders a div with inline styles with client render on top of good server markup -* renders a div with inline styles with client render on top of bad server markup * renders a self-closing tag with server string render * renders a self-closing tag with server stream render * renders a self-closing tag with clean client render @@ -930,6 +912,11 @@ src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js * renders string prop with false value with clean client render * renders string prop with false value with client render on top of good server markup * renders string prop with false value with client render on top of bad server markup +* renders no string prop with null value with server string render +* renders no string prop with null value with server stream render +* renders no string prop with null value with clean client render +* renders no string prop with null value with client render on top of good server markup +* renders no string prop with null value with client render on top of bad server markup * renders boolean prop with true value with server string render * renders boolean prop with true value with server stream render * renders boolean prop with true value with clean client render @@ -975,6 +962,11 @@ src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js * renders boolean prop with zero value with clean client render * renders boolean prop with zero value with client render on top of good server markup * renders boolean prop with zero value with client render on top of bad server markup +* renders no boolean prop with null value with server string render +* renders no boolean prop with null value with server stream render +* renders no boolean prop with null value with clean client render +* renders no boolean prop with null value with client render on top of good server markup +* renders no boolean prop with null value with client render on top of bad server markup * renders download prop with true value with server string render * renders download prop with true value with server stream render * renders download prop with true value with clean client render @@ -990,11 +982,31 @@ src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js * renders download prop with string value with clean client render * renders download prop with string value with client render on top of good server markup * renders download prop with string value with client render on top of bad server markup +* renders download prop with string "false" value with server string render +* renders download prop with string "false" value with server stream render +* renders download prop with string "false" value with clean client render +* renders download prop with string "false" value with client render on top of good server markup +* renders download prop with string "false" value with client render on top of bad server markup * renders download prop with string "true" value with server string render * renders download prop with string "true" value with server stream render * renders download prop with string "true" value with clean client render * renders download prop with string "true" value with client render on top of good server markup * renders download prop with string "true" value with client render on top of bad server markup +* renders download prop with number 0 value with server string render +* renders download prop with number 0 value with server stream render +* renders download prop with number 0 value with clean client render +* renders download prop with number 0 value with client render on top of good server markup +* renders download prop with number 0 value with client render on top of bad server markup +* renders no download prop with null value with server string render +* renders no download prop with null value with server stream render +* renders no download prop with null value with clean client render +* renders no download prop with null value with client render on top of good server markup +* renders no download prop with null value with client render on top of bad server markup +* renders no download prop with undefined value with server string render +* renders no download prop with undefined value with server stream render +* renders no download prop with undefined value with clean client render +* renders no download prop with undefined value with client render on top of good server markup +* renders no download prop with undefined value with client render on top of bad server markup * renders className prop with string value with server string render * renders className prop with string value with server stream render * renders className prop with string value with clean client render @@ -1015,6 +1027,11 @@ src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js * renders className prop with false value with clean client render * renders className prop with false value with client render on top of good server markup * renders className prop with false value with client render on top of bad server markup +* renders no className prop with null value with server string render +* renders no className prop with null value with server stream render +* renders no className prop with null value with clean client render +* renders no className prop with null value with client render on top of good server markup +* renders no className prop with null value with client render on top of bad server markup * renders htmlFor with string value with server string render * renders htmlFor with string value with server stream render * renders htmlFor with string value with clean client render @@ -1025,16 +1042,36 @@ src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js * renders htmlFor with an empty string with clean client render * renders htmlFor with an empty string with client render on top of good server markup * renders htmlFor with an empty string with client render on top of bad server markup -* renders className prop with true value with server string render -* renders className prop with true value with server stream render -* renders className prop with true value with clean client render -* renders className prop with true value with client render on top of good server markup -* renders className prop with true value with client render on top of bad server markup -* renders className prop with false value with server string render -* renders className prop with false value with server stream render -* renders className prop with false value with clean client render -* renders className prop with false value with client render on top of good server markup -* renders className prop with false value with client render on top of bad server markup +* renders htmlFor prop with true value with server string render +* renders htmlFor prop with true value with server stream render +* renders htmlFor prop with true value with clean client render +* renders htmlFor prop with true value with client render on top of good server markup +* renders htmlFor prop with true value with client render on top of bad server markup +* renders htmlFor prop with false value with server string render +* renders htmlFor prop with false value with server stream render +* renders htmlFor prop with false value with clean client render +* renders htmlFor prop with false value with client render on top of good server markup +* renders htmlFor prop with false value with client render on top of bad server markup +* renders no htmlFor prop with null value with server string render +* renders no htmlFor prop with null value with server stream render +* renders no htmlFor prop with null value with clean client render +* renders no htmlFor prop with null value with client render on top of good server markup +* renders no htmlFor prop with null value with client render on top of bad server markup +* renders positive numeric property with positive value with server string render +* renders positive numeric property with positive value with server stream render +* renders positive numeric property with positive value with clean client render +* renders positive numeric property with positive value with client render on top of good server markup +* renders positive numeric property with positive value with client render on top of bad server markup +* renders no positive numeric property with zero value with server string render +* renders no positive numeric property with zero value with server stream render +* renders no positive numeric property with zero value with clean client render +* renders no positive numeric property with zero value with client render on top of good server markup +* renders no positive numeric property with zero value with client render on top of bad server markup +* renders numeric property with zero value with server string render +* renders numeric property with zero value with server stream render +* renders numeric property with zero value with clean client render +* renders numeric property with zero value with client render on top of good server markup +* renders numeric property with zero value with client render on top of bad server markup * renders no ref attribute with server string render * renders no ref attribute with server stream render * renders no ref attribute with clean client render @@ -1055,6 +1092,51 @@ src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js * renders no dangerouslySetInnerHTML attribute with clean client render * renders no dangerouslySetInnerHTML attribute with client render on top of good server markup * renders no dangerouslySetInnerHTML attribute with client render on top of bad server markup +* renders simple styles with server string render +* renders simple styles with server stream render +* renders simple styles with clean client render +* renders simple styles with client render on top of good server markup +* renders simple styles with client render on top of bad server markup +* renders relevant styles with px with server string render +* renders relevant styles with px with server stream render +* renders relevant styles with px with clean client render +* renders relevant styles with px with client render on top of good server markup +* renders relevant styles with px with client render on top of bad server markup +* renders custom properties with server string render +* renders custom properties with server stream render +* renders custom properties with clean client render +* renders custom properties with client render on top of good server markup +* renders custom properties with client render on top of bad server markup +* renders no undefined styles with server string render +* renders no undefined styles with server stream render +* renders no undefined styles with clean client render +* renders no undefined styles with client render on top of good server markup +* renders no undefined styles with client render on top of bad server markup +* renders no null styles with server string render +* renders no null styles with server stream render +* renders no null styles with clean client render +* renders no null styles with client render on top of good server markup +* renders no null styles with client render on top of bad server markup +* renders no empty styles with server string render +* renders no empty styles with server stream render +* renders no empty styles with clean client render +* renders no empty styles with client render on top of good server markup +* renders no empty styles with client render on top of bad server markup +* renders simple strings with server string render +* renders simple strings with server stream render +* renders simple strings with clean client render +* renders simple strings with client render on top of good server markup +* renders simple strings with client render on top of bad server markup +* renders aria string prop with false value with server string render +* renders aria string prop with false value with server stream render +* renders aria string prop with false value with clean client render +* renders aria string prop with false value with client render on top of good server markup +* renders aria string prop with false value with client render on top of bad server markup +* renders no aria prop with null value with server string render +* renders no aria prop with null value with server stream render +* renders no aria prop with null value with clean client render +* renders no aria prop with null value with client render on top of good server markup +* renders no aria prop with null value with client render on top of bad server markup * renders no unknown attributes with server string render * renders no unknown attributes with server stream render * renders no unknown attributes with clean client render @@ -1065,6 +1147,11 @@ src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js * renders unknown data- attributes with clean client render * renders unknown data- attributes with client render on top of good server markup * renders unknown data- attributes with client render on top of bad server markup +* renders no unknown data- attributes with null value with server string render +* renders no unknown data- attributes with null value with server stream render +* renders no unknown data- attributes with null value with clean client render +* renders no unknown data- attributes with null value with client render on top of good server markup +* renders no unknown data- attributes with null value with client render on top of bad server markup * renders no unknown attributes for non-standard elements with server string render * renders no unknown attributes for non-standard elements with server stream render * renders no unknown attributes for non-standard elements with clean client render @@ -1075,11 +1162,21 @@ src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js * renders unknown attributes for custom elements with clean client render * renders unknown attributes for custom elements with client render on top of good server markup * renders unknown attributes for custom elements with client render on top of bad server markup +* renders no unknown attributes for custom elements with null value with server string render +* renders no unknown attributes for custom elements with null value with server stream render +* renders no unknown attributes for custom elements with null value with clean client render +* renders no unknown attributes for custom elements with null value with client render on top of good server markup +* renders no unknown attributes for custom elements with null value with client render on top of bad server markup * renders unknown attributes for custom elements using is with server string render * renders unknown attributes for custom elements using is with server stream render * renders unknown attributes for custom elements using is with clean client render * renders unknown attributes for custom elements using is with client render on top of good server markup * renders unknown attributes for custom elements using is with client render on top of bad server markup +* renders no unknown attributes for custom elements using is with null value with server string render +* renders no unknown attributes for custom elements using is with null value with server stream render +* renders no unknown attributes for custom elements using is with null value with clean client render +* renders no unknown attributes for custom elements using is with null value with client render on top of good server markup +* renders no unknown attributes for custom elements using is with null value with client render on top of bad server markup * renders no HTML events with server string render * renders no HTML events with server stream render * renders no HTML events with clean client render @@ -1563,6 +1660,11 @@ src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js * can distinguish an empty component from a dom node * can distinguish an empty component from an empty text component * should error reconnecting a div with different dangerouslySetInnerHTML +* renders injected attributes with server string render +* renders injected attributes with server stream render +* renders injected attributes with clean client render +* renders injected attributes with client render on top of good server markup +* renders injected attributes with client render on top of bad server markup src/renderers/dom/shared/__tests__/ReactDOMTextComponent-test.js * updates a mounted text component in place diff --git a/scripts/rollup/results.json b/scripts/rollup/results.json index 185922450e083..34f5166af3ba5 100644 --- a/scripts/rollup/results.json +++ b/scripts/rollup/results.json @@ -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, diff --git a/src/renderers/dom/shared/CSSPropertyOperations.js b/src/renderers/dom/shared/CSSPropertyOperations.js index 8e285f5eea6b4..25f95ef353f8c 100644 --- a/src/renderers/dom/shared/CSSPropertyOperations.js +++ b/src/renderers/dom/shared/CSSPropertyOperations.js @@ -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__) { diff --git a/src/renderers/dom/shared/DOMMarkupOperations.js b/src/renderers/dom/shared/DOMMarkupOperations.js new file mode 100644 index 0000000000000..1f34e0d0fefc3 --- /dev/null +++ b/src/renderers/dom/shared/DOMMarkupOperations.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; diff --git a/src/renderers/dom/shared/DOMPropertyOperations.js b/src/renderers/dom/shared/DOMPropertyOperations.js index 94b751e67dde5..5a4a7613aa625 100644 --- a/src/renderers/dom/shared/DOMPropertyOperations.js +++ b/src/renderers/dom/shared/DOMPropertyOperations.js @@ -15,9 +15,10 @@ var DOMProperty = require('DOMProperty'); var ReactDOMComponentTree = require('ReactDOMComponentTree'); var ReactInstrumentation = require('ReactInstrumentation'); -var quoteAttributeValueForBrowser = require('quoteAttributeValueForBrowser'); var warning = require('fbjs/lib/warning'); +// isAttributeNameSafe() is currently duplicated in DOMMarkupOperations. +// TODO: Find a better place for this. var VALID_ATTRIBUTE_NAME_REGEX = new RegExp( '^[' + DOMProperty.ATTRIBUTE_NAME_START_CHAR + @@ -27,7 +28,6 @@ var VALID_ATTRIBUTE_NAME_REGEX = new RegExp( ); var illegalAttributeNameCache = {}; var validatedAttributeNameCache = {}; - function isAttributeNameSafe(attributeName) { if (validatedAttributeNameCache.hasOwnProperty(attributeName)) { return true; @@ -44,6 +44,8 @@ function isAttributeNameSafe(attributeName) { return false; } +// shouldIgnoreValue() is currently duplicated in DOMMarkupOperations. +// TODO: Find a better place for this. function shouldIgnoreValue(propertyInfo, value) { return ( value == null || @@ -58,76 +60,14 @@ function shouldIgnoreValue(propertyInfo, value) { * Operations for dealing with DOM properties. */ var DOMPropertyOperations = { - /** - * 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) - ); - }, - setAttributeForID: function(node, id) { node.setAttribute(DOMProperty.ID_ATTRIBUTE_NAME, id); }, - createMarkupForRoot: function() { - return DOMProperty.ROOT_ATTRIBUTE_NAME + '=""'; - }, - setAttributeForRoot: function(node) { node.setAttribute(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); - }, - /** * Get the value for a property on a node. Only used in DEV for SSR validation. * The "expected" argument is used as a hint of what the expected value is. @@ -153,6 +93,12 @@ var DOMPropertyOperations = { if (value === '') { return true; } + if (shouldIgnoreValue(propertyInfo, expected)) { + return value; + } + if (value === '' + expected) { + return expected; + } return value; } } else if (node.hasAttribute(attributeName)) { diff --git a/src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js b/src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js index 065ac059dccbe..2e3f9fc6038ac 100644 --- a/src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js +++ b/src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js @@ -16,97 +16,47 @@ var ReactDOM = require('react-dom'); var ReactDOMServer = require('react-dom/server'); describe('CSSPropertyOperations', () => { - var CSSPropertyOperations; - - beforeEach(() => { - jest.resetModules(); - CSSPropertyOperations = require('CSSPropertyOperations'); - }); - - it('should create markup for simple styles', () => { - expect( - CSSPropertyOperations.createMarkupForStyles({ - backgroundColor: '#3b5998', - display: 'none', - }), - ).toBe('background-color:#3b5998;display:none'); - }); - - it('should ignore undefined styles', () => { - expect( - CSSPropertyOperations.createMarkupForStyles({ - backgroundColor: undefined, - display: 'none', - }), - ).toBe('display:none'); - }); - - it('should ignore null styles', () => { - expect( - CSSPropertyOperations.createMarkupForStyles({ - backgroundColor: null, - display: 'none', - }), - ).toBe('display:none'); - }); - - it('should return null for no styles', () => { - expect( - CSSPropertyOperations.createMarkupForStyles({ - backgroundColor: null, - display: null, - }), - ).toBe(null); - }); - it('should automatically append `px` to relevant styles', () => { - expect( - CSSPropertyOperations.createMarkupForStyles({ - left: 0, - margin: 16, - opacity: 0.5, - padding: '4px', - }), - ).toBe('left:0;margin:16px;opacity:0.5;padding:4px'); + var styles = { + left: 0, + margin: 16, + opacity: 0.5, + padding: '4px', + }; + var div =
; + var html = ReactDOMServer.renderToString(div); + expect(html).toContain('"left:0;margin:16px;opacity:0.5;padding:4px"'); }); it('should trim values', () => { - expect( - CSSPropertyOperations.createMarkupForStyles({ - left: '16 ', - opacity: 0.5, - right: ' 4 ', - }), - ).toBe('left:16;opacity:0.5;right:4'); + var styles = { + left: '16 ', + opacity: 0.5, + right: ' 4 ', + }; + var div =
; + var html = ReactDOMServer.renderToString(div); + expect(html).toContain('"left:16;opacity:0.5;right:4"'); }); it('should not append `px` to styles that might need a number', () => { - var CSSProperty = require('CSSProperty'); - var unitlessProperties = Object.keys(CSSProperty.isUnitlessNumber); - unitlessProperties.forEach(function(property) { - var styles = {}; - styles[property] = 1; - expect(CSSPropertyOperations.createMarkupForStyles(styles)).toMatch( - /:1$/, - ); - }); + var styles = { + flex: 0, + opacity: 0.5, + }; + var div =
; + var html = ReactDOMServer.renderToString(div); + expect(html).toContain('"flex:0;opacity:0.5"'); }); it('should create vendor-prefixed markup correctly', () => { - expect( - CSSPropertyOperations.createMarkupForStyles({ - msTransition: 'none', - MozTransition: 'none', - }), - ).toBe('-ms-transition:none;-moz-transition:none'); - }); - - it('should create markup with unitless css custom property', () => { - expect( - CSSPropertyOperations.createMarkupForStyles({ - '--foo': 5, - }), - ).toBe('--foo:5'); + var styles = { + msTransition: 'none', + MozTransition: 'none', + }; + var div =
; + var html = ReactDOMServer.renderToString(div); + expect(html).toContain('"-ms-transition:none;-moz-transition:none"'); }); it('should set style attribute when styles exist', () => { diff --git a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js index 954780de8f2da..eba36070cb1dc 100644 --- a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js +++ b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js @@ -26,122 +26,6 @@ describe('DOMPropertyOperations', () => { ReactDOMComponentTree = require('ReactDOMComponentTree'); }); - describe('createMarkupForProperty', () => { - it('should create markup for simple properties', () => { - expect( - DOMPropertyOperations.createMarkupForProperty('name', 'simple'), - ).toBe('name="simple"'); - - expect(DOMPropertyOperations.createMarkupForProperty('name', false)).toBe( - 'name="false"', - ); - - expect(DOMPropertyOperations.createMarkupForProperty('name', null)).toBe( - '', - ); - }); - - it('should work with the id attribute', () => { - expect( - DOMPropertyOperations.createMarkupForProperty('id', 'simple'), - ).toBe('id="simple"'); - }); - - it('should create markup for boolean properties', () => { - expect( - DOMPropertyOperations.createMarkupForProperty('checked', 'simple'), - ).toBe('checked=""'); - - expect( - DOMPropertyOperations.createMarkupForProperty('checked', true), - ).toBe('checked=""'); - - expect( - DOMPropertyOperations.createMarkupForProperty('checked', false), - ).toBe(''); - - expect( - DOMPropertyOperations.createMarkupForProperty('scoped', true), - ).toBe('scoped=""'); - }); - - it('should create markup for booleanish properties', () => { - expect( - DOMPropertyOperations.createMarkupForProperty('download', 'simple'), - ).toBe('download="simple"'); - - expect( - DOMPropertyOperations.createMarkupForProperty('download', true), - ).toBe('download=""'); - - expect( - DOMPropertyOperations.createMarkupForProperty('download', 'true'), - ).toBe('download="true"'); - - expect( - DOMPropertyOperations.createMarkupForProperty('download', false), - ).toBe(''); - - expect( - DOMPropertyOperations.createMarkupForProperty('download', 'false'), - ).toBe('download="false"'); - - expect( - DOMPropertyOperations.createMarkupForProperty('download', undefined), - ).toBe(''); - - expect( - DOMPropertyOperations.createMarkupForProperty('download', null), - ).toBe(''); - - expect(DOMPropertyOperations.createMarkupForProperty('download', 0)).toBe( - 'download="0"', - ); - }); - - it('should create markup for custom attributes', () => { - expect( - DOMPropertyOperations.createMarkupForProperty('aria-label', 'simple'), - ).toBe('aria-label="simple"'); - - expect( - DOMPropertyOperations.createMarkupForProperty('aria-label', false), - ).toBe('aria-label="false"'); - - expect( - DOMPropertyOperations.createMarkupForProperty('aria-label', null), - ).toBe(''); - }); - - it('should create markup for numeric properties', () => { - expect(DOMPropertyOperations.createMarkupForProperty('start', 5)).toBe( - 'start="5"', - ); - - expect(DOMPropertyOperations.createMarkupForProperty('start', 0)).toBe( - 'start="0"', - ); - - expect(DOMPropertyOperations.createMarkupForProperty('size', 0)).toBe(''); - - expect(DOMPropertyOperations.createMarkupForProperty('size', 1)).toBe( - 'size="1"', - ); - }); - }); - - describe('createMarkupForProperty', () => { - it('should allow custom properties on web components', () => { - expect( - DOMPropertyOperations.createMarkupForCustomAttribute('awesomeness', 5), - ).toBe('awesomeness="5"'); - - expect( - DOMPropertyOperations.createMarkupForCustomAttribute('dev', 'jim'), - ).toBe('dev="jim"'); - }); - }); - describe('setValueForProperty', () => { var stubNode; var stubInstance; @@ -377,51 +261,4 @@ describe('DOMPropertyOperations', () => { ); }); }); - - describe('injectDOMPropertyConfig', () => { - it('should support custom attributes', () => { - // foobar does not exist yet - expect( - DOMPropertyOperations.createMarkupForProperty('foobar', 'simple'), - ).toBe(null); - - // foo-* does not exist yet - expect( - DOMPropertyOperations.createMarkupForProperty('foo-xyz', 'simple'), - ).toBe(null); - - // inject foobar DOM property - DOMProperty.injection.injectDOMPropertyConfig({ - isCustomAttribute: function(name) { - return name.indexOf('foo-') === 0; - }, - Properties: {foobar: null}, - }); - - // Ensure old attributes still work - expect( - DOMPropertyOperations.createMarkupForProperty('name', 'simple'), - ).toBe('name="simple"'); - expect( - DOMPropertyOperations.createMarkupForProperty('data-name', 'simple'), - ).toBe('data-name="simple"'); - - // foobar should work - expect( - DOMPropertyOperations.createMarkupForProperty('foobar', 'simple'), - ).toBe('foobar="simple"'); - - // foo-* should work - expect( - DOMPropertyOperations.createMarkupForProperty('foo-xyz', 'simple'), - ).toBe('foo-xyz="simple"'); - - // It should complain about double injections. - expect(function() { - DOMProperty.injection.injectDOMPropertyConfig({ - Properties: {foobar: null}, - }); - }).toThrow(); - }); - }); }); diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index 08fc0ebe95c83..220c71f9aba70 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -285,6 +285,7 @@ function expectMarkupMismatch(serverElement, clientElement) { // error, you want to see the error show up both on server and client. Unfortunately, // React refuses to issue the same error twice to avoid clogging up the console. // To get around this, we must reload React modules in between server and client render. +let onAfterResetModules = null; function resetModules() { jest.resetModuleRegistry(); PropTypes = require('prop-types'); @@ -296,6 +297,12 @@ function resetModules() { ReactTestUtils = require('react-dom/test-utils'); // TODO: can we express this test with only public API? ExecutionEnvironment = require('ExecutionEnvironment'); + + // TODO: this is a hack for testing dynamic injection. Remove this when we decide + // how to do static injection instead. + if (typeof onAfterResetModules === 'function') { + onAfterResetModules(); + } } describe('ReactDOMServerIntegration', () => { @@ -311,12 +318,6 @@ describe('ReactDOMServerIntegration', () => { expect(e.tagName).toBe('DIV'); }); - itRenders('a div with inline styles', async render => { - const e = await render(
); - expect(e.style.color).toBe('red'); - expect(e.style.width).toBe('30px'); - }); - itRenders('a self-closing tag', async render => { const e = await render(
); expect(e.tagName).toBe('BR'); @@ -352,6 +353,11 @@ describe('ReactDOMServerIntegration', () => { const e = await render(); expect(e.getAttribute('href')).toBe('false'); }); + + itRenders('no string prop with null value', async render => { + const e = await render(
); + expect(e.hasAttribute('width')).toBe(false); + }); }); describe('boolean properties', function() { @@ -407,6 +413,11 @@ describe('ReactDOMServerIntegration', () => { const e = await render(