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`.