Set up `style` mutation monitoring
This works by storing the style and its copy onto the component, and do the appropriate comparison during `_updateDOMProperties`.
Cheng Lou committed
Mar 16, 2015 at 03:56 UTC
6e992fbbddbdcafdd9510897a770f4ef913f7555
2 files changed
+97
src/browser/ui/ReactDOMComponent.js
+52
@@ -29,6 +29,7 @@ var escapeTextContentForBrowser = require('escapeTextContentForBrowser');
29
var invariant = require('invariant');
30
var isEventSupported = require('isEventSupported');
31
var keyOf = require('keyOf');
32
+var shallowEqual = require('shallowEqual');
33
var validateDOMNesting = require('validateDOMNesting');
34
var warning = require('warning');
35
@@ -43,6 +44,44 @@ var STYLE = keyOf({style: null});
44
45
var ELEMENT_NODE_TYPE = 1;
46
47
+var styleMutationWarning = {};
48
+
49
+function checkAndWarnForMutatedStyle(style1, style2, component) {
50
+ if (style1 == null || style2 == null) {
51
+ return;
52
+ }
53
+ if (shallowEqual(style1, style2)) {
54
+ return;
55
+ }
56
+
57
+ var componentName = component._tag;
58
+ var owner = component._currentElement._owner;
59
+ var ownerName;
60
+ if (owner) {
61
+ ownerName = owner.getName();
62
+ }
63
+
64
+ var hash = ownerName + '|' + componentName;
65
+
66
+ if (styleMutationWarning.hasOwnProperty(hash)) {
67
+ return;
68
+ }
69
+
70
+ styleMutationWarning[hash] = true;
71
+
72
+ warning(
73
+ false,
74
+ '`%s` was passed a style object that has previously been mutated. ' +
75
+ 'Mutating `style` will be deprecated. Consider cloning it beforehand. ' +
76
+ 'Check the `render` %s. Previous style: %s. Mutated style: %s.',
77
+ componentName,
78
+ owner ? 'of `' + ownerName + '`' : 'using <' + componentName + '>',
79
+ JSON.stringify(style1),
80
+ JSON.stringify(style2)
81
+ );
82
+}
83
+
84
+
85
/**
86
* Optionally injectable operations for mutating the DOM
87
*/
@@ -201,6 +240,7 @@ function ReactDOMComponent(tag) {
240
validateDangerousTag(tag);
241
this._tag = tag;
242
this._renderedChildren = null;
243
+ this._previousStyle = null;
244
this._previousStyleCopy = null;
245
this._rootNodeID = null;
246
}
@@ -274,6 +314,10 @@ ReactDOMComponent.Mixin = {
314
} else {
315
if (propKey === STYLE) {
316
if (propValue) {
317
+ if (__DEV__) {
318
+ // See `_updateDOMProperties`. style block
319
+ this._previousStyle = propValue;
320
+ }
321
propValue = this._previousStyleCopy = assign({}, props.style);
322
}
323
propValue = CSSPropertyOperations.createMarkupForStyles(propValue);
@@ -441,6 +485,14 @@ ReactDOMComponent.Mixin = {
485
}
486
if (propKey === STYLE) {
487
if (nextProp) {
488
+ if (__DEV__) {
489
+ checkAndWarnForMutatedStyle(
490
+ this._previousStyleCopy,
491
+ this._previousStyle,
492
+ this
493
+ );
494
+ this._previousStyle = nextProp;
495
+ }
496
nextProp = this._previousStyleCopy = assign({}, nextProp);
497
}
498
if (lastProp) {
src/browser/ui/__tests__/ReactDOMComponent-test.js
+45
@@ -60,7 +60,11 @@ describe('ReactDOMComponent', function() {
60
expect(stubStyle.fontFamily).toEqual('');
61
});
62
63
+ // TODO: (poshannessy) deprecate this pattern.
64
it("should update styles when mutating style object", function() {
65
+ // not actually used. Just to suppress the style mutation warning
66
+ spyOn(console, 'error');
67
+
68
var styles = {display: 'none', fontFamily: 'Arial', lineHeight: 1.2};
69
var container = document.createElement('div');
70
React.render(<div style={styles} />, container);
@@ -96,6 +100,46 @@ describe('ReactDOMComponent', function() {
100
expect(stubStyle.lineHeight).toBe('');
101
});
102
103
+ it('should warn when mutating style', function() {
104
+ spyOn(console, 'error');
105
+
106
+ var style = {border: '1px solid black'};
107
+ var App = React.createClass({
108
+ getInitialState: function() {
109
+ return {style: style};
110
+ },
111
+ render: function() {
112
+ return <div style={this.state.style}>asd</div>;
113
+ }
114
+ });
115
+
116
+ var stub = ReactTestUtils.renderIntoDocument(<App />);
117
+ style.position = 'absolute';
118
+ stub.setState({style: style});
119
+ expect(console.error.argsForCall.length).toBe(1);
120
+ expect(console.error.argsForCall[0][0]).toEqual(
121
+ 'Warning: `div` was passed a style object that has previously been ' +
122
+ 'mutated. Mutating `style` will be deprecated. Consider cloning it ' +
123
+ 'beforehand. Check the `render` of `App`. Previous style: ' +
124
+ '{"border":"1px solid black"}. Mutated style: ' +
125
+ '{"border":"1px solid black","position":"absolute"}.'
126
+ );
127
+
128
+ style = {background: 'red'};
129
+ stub = ReactTestUtils.renderIntoDocument(<App />);
130
+ style.background = 'green';
131
+ stub.setState({style: {background: 'green'}});
132
+ // already warned once for the same component and owner
133
+ expect(console.error.argsForCall.length).toBe(1);
134
+
135
+ style = {background: 'red'};
136
+ var div = document.createElement('div');
137
+ React.render(<span style={style}></span>, div);
138
+ style.background = 'blue';
139
+ React.render(<span style={style}></span>, div);
140
+ expect(console.error.argsForCall.length).toBe(2);
141
+ });
142
+
143
it("should update styles if initially null", function() {
144
var styles = null;
145
var container = document.createElement('div');
@@ -152,6 +196,7 @@ describe('ReactDOMComponent', function() {
196
expect(stubStyle.color).toEqual('');
197
});
198
199
+ // trouble
200
it("should update styles when 'style' changes from null to object", function() {
201
var container = document.createElement('div');
202
var styles = {color: 'red'};