From 977357765b44af8ff0cfea327866861073095c12 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 20 Nov 2016 19:51:02 +0000 Subject: [PATCH] Drop "previous style" copy from Stack to bring it inline with Fiber We've already been warning for mutating styles so now we'll freeze them in DEV instead. This isn't as good because we don't have more specific warnings than the generic error that doesn't even fire in sloppy mode. This lets Fiber pass the same tests. --- scripts/fiber/tests-failing.txt | 2 - scripts/fiber/tests-passing.txt | 2 + .../__tests__/ReactDOMComponent-test.js | 73 +++++++--------- .../dom/stack/client/ReactDOMComponent.js | 87 +------------------ 4 files changed, 39 insertions(+), 125 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index fb1989207af8f..d3b2d9cefba95 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -25,8 +25,6 @@ src/renderers/art/__tests__/ReactART-test.js * resolves refs before componentDidUpdate src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js -* should update styles when mutating style object -* should warn when mutating style * should empty element when removing innerHTML * should transition from innerHTML to children in nested el * should warn for children on void elements diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index c4fef3106349c..3ddd25a90b049 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -560,6 +560,8 @@ src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should handle className * should gracefully handle various style value types +* should not update styles when mutating a proxy style object +* should throw when mutating style objectsd * should not warn for "0" as a unitless style value * should warn nicely about NaN in style * should update styles if initially null diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index c3be319d548f8..a9fa96b116061 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -72,12 +72,30 @@ describe('ReactDOMComponent', () => { expect(stubStyle.fontFamily).toEqual(''); }); - // TODO: (poshannessy) deprecate this pattern. - it('should update styles when mutating style object', () => { - // not actually used. Just to suppress the style mutation warning - spyOn(console, 'error'); - - var styles = {display: 'none', fontFamily: 'Arial', lineHeight: 1.2}; + it('should not update styles when mutating a proxy style object', () => { + var styleStore = {display: 'none', fontFamily: 'Arial', lineHeight: 1.2}; + // We use a proxy style object so that we can mutate it even if it is + // frozen in DEV. + var styles = { + get display() { + return styleStore.display; + }, + set display(v) { + styleStore.display = v; + }, + get fontFamily() { + return styleStore.fontFamily; + }, + set fontFamily(v) { + styleStore.fontFamily = v; + }, + get lineHeight() { + return styleStore.lineHeight; + }, + set lineHeight(v) { + styleStore.lineHeight = v; + }, + }; var container = document.createElement('div'); ReactDOM.render(
, container); @@ -88,23 +106,23 @@ describe('ReactDOMComponent', () => { styles.display = 'block'; ReactDOM.render(
, container); - expect(stubStyle.display).toEqual('block'); + expect(stubStyle.display).toEqual('none'); expect(stubStyle.fontFamily).toEqual('Arial'); expect(stubStyle.lineHeight).toEqual('1.2'); styles.fontFamily = 'Helvetica'; ReactDOM.render(
, container); - expect(stubStyle.display).toEqual('block'); - expect(stubStyle.fontFamily).toEqual('Helvetica'); + expect(stubStyle.display).toEqual('none'); + expect(stubStyle.fontFamily).toEqual('Arial'); expect(stubStyle.lineHeight).toEqual('1.2'); styles.lineHeight = 0.5; ReactDOM.render(
, container); - expect(stubStyle.display).toEqual('block'); - expect(stubStyle.fontFamily).toEqual('Helvetica'); - expect(stubStyle.lineHeight).toEqual('0.5'); + expect(stubStyle.display).toEqual('none'); + expect(stubStyle.fontFamily).toEqual('Arial'); + expect(stubStyle.lineHeight).toEqual('1.2'); ReactDOM.render(
, container); expect(stubStyle.display).toBe(''); @@ -112,9 +130,7 @@ describe('ReactDOMComponent', () => { expect(stubStyle.lineHeight).toBe(''); }); - it('should warn when mutating style', () => { - spyOn(console, 'error'); - + it('should throw when mutating style objectsd', () => { var style = {border: '1px solid black'}; class App extends React.Component { @@ -125,31 +141,8 @@ describe('ReactDOMComponent', () => { } } - var stub = ReactTestUtils.renderIntoDocument(); - style.position = 'absolute'; - stub.setState({style: style}); - expectDev(console.error.calls.count()).toBe(1); - expectDev(console.error.calls.argsFor(0)[0]).toEqual( - 'Warning: `div` was passed a style object that has previously been ' + - 'mutated. Mutating `style` is deprecated. Consider cloning it ' + - 'beforehand. Check the `render` of `App`. Previous style: ' + - '{border: "1px solid black"}. Mutated style: ' + - '{border: "1px solid black", position: "absolute"}.' - ); - - style = {background: 'red'}; - stub = ReactTestUtils.renderIntoDocument(); - style.background = 'green'; - stub.setState({style: {background: 'green'}}); - // already warned once for the same component and owner - expectDev(console.error.calls.count()).toBe(1); - - style = {background: 'red'}; - var div = document.createElement('div'); - ReactDOM.render(, div); - style.background = 'blue'; - ReactDOM.render(, div); - expectDev(console.error.calls.count()).toBe(2); + ReactTestUtils.renderIntoDocument(); + expectDev(() => style.position = 'absolute').toThrow(); }); it('should warn for unknown prop', () => { diff --git a/src/renderers/dom/stack/client/ReactDOMComponent.js b/src/renderers/dom/stack/client/ReactDOMComponent.js index cebcc047e8c23..0992b42f086fd 100644 --- a/src/renderers/dom/stack/client/ReactDOMComponent.js +++ b/src/renderers/dom/stack/client/ReactDOMComponent.js @@ -35,7 +35,6 @@ var emptyFunction = require('emptyFunction'); var escapeTextContentForBrowser = require('escapeTextContentForBrowser'); var invariant = require('invariant'); var isEventSupported = require('isEventSupported'); -var shallowEqual = require('shallowEqual'); var inputValueTracking = require('inputValueTracking'); var validateDOMNesting = require('validateDOMNesting'); var warning = require('warning'); @@ -74,69 +73,6 @@ function getDeclarationErrorAddendum(internalInstance) { return ''; } -function friendlyStringify(obj) { - if (typeof obj === 'object') { - if (Array.isArray(obj)) { - return '[' + obj.map(friendlyStringify).join(', ') + ']'; - } else { - var pairs = []; - for (var key in obj) { - if (Object.prototype.hasOwnProperty.call(obj, key)) { - var keyEscaped = /^[a-z$_][\w$_]*$/i.test(key) ? - key : - JSON.stringify(key); - pairs.push(keyEscaped + ': ' + friendlyStringify(obj[key])); - } - } - return '{' + pairs.join(', ') + '}'; - } - } else if (typeof obj === 'string') { - return JSON.stringify(obj); - } else if (typeof obj === 'function') { - return '[function object]'; - } - // Differs from JSON.stringify in that undefined because undefined and that - // inf and nan don't become null - return String(obj); -} - -var styleMutationWarning = {}; - -function checkAndWarnForMutatedStyle(style1, style2, component) { - if (style1 == null || style2 == null) { - return; - } - if (shallowEqual(style1, style2)) { - return; - } - - var componentName = component._tag; - var owner = component._currentElement._owner; - var ownerName; - if (owner) { - ownerName = owner.getName(); - } - - var hash = ownerName + '|' + componentName; - - if (styleMutationWarning.hasOwnProperty(hash)) { - return; - } - - styleMutationWarning[hash] = true; - - warning( - false, - '`%s` was passed a style object that has previously been mutated. ' + - 'Mutating `style` is deprecated. Consider cloning it beforehand. Check ' + - 'the `render` %s. Previous style: %s. Mutated style: %s.', - componentName, - owner ? 'of `' + ownerName + '`' : 'using <' + componentName + '>', - friendlyStringify(style1), - friendlyStringify(style2) - ); -} - /** * @param {object} component * @param {?object} props @@ -482,8 +418,6 @@ function ReactDOMComponent(element) { this._tag = tag.toLowerCase(); this._namespaceURI = null; this._renderedChildren = null; - this._previousStyle = null; - this._previousStyleCopy = null; this._hostNode = null; this._hostParent = null; this._rootNodeID = 0; @@ -763,10 +697,8 @@ ReactDOMComponent.Mixin = { if (propKey === STYLE) { if (propValue) { if (__DEV__) { - // See `_updateDOMProperties`. style block - this._previousStyle = propValue; + Object.freeze(propValue); } - propValue = this._previousStyleCopy = Object.assign({}, props.style); } propValue = CSSPropertyOperations.createMarkupForStyles(propValue, this); } @@ -1003,14 +935,13 @@ ReactDOMComponent.Mixin = { continue; } if (propKey === STYLE) { - var lastStyle = this._previousStyleCopy; + var lastStyle = lastProps[STYLE]; for (styleName in lastStyle) { if (lastStyle.hasOwnProperty(styleName)) { styleUpdates = styleUpdates || {}; styleUpdates[styleName] = ''; } } - this._previousStyleCopy = null; } else if (registrationNameModules.hasOwnProperty(propKey)) { // Do nothing for event names. } else if (isCustomComponent(this._tag, lastProps)) { @@ -1028,9 +959,7 @@ ReactDOMComponent.Mixin = { } for (propKey in nextProps) { var nextProp = nextProps[propKey]; - var lastProp = - propKey === STYLE ? this._previousStyleCopy : - lastProps != null ? lastProps[propKey] : undefined; + var lastProp = lastProps != null ? lastProps[propKey] : undefined; if (!nextProps.hasOwnProperty(propKey) || nextProp === lastProp || nextProp == null && lastProp == null) { @@ -1039,16 +968,8 @@ ReactDOMComponent.Mixin = { if (propKey === STYLE) { if (nextProp) { if (__DEV__) { - checkAndWarnForMutatedStyle( - this._previousStyleCopy, - this._previousStyle, - this - ); - this._previousStyle = nextProp; + Object.freeze(nextProp); } - nextProp = this._previousStyleCopy = Object.assign({}, nextProp); - } else { - this._previousStyleCopy = null; } if (lastProp) { // Unset styles on `lastProp` but not on `nextProp`.