Skip to content

Commit

Permalink
Drop "previous style" copy from Stack to bring it inline with Fiber
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@sebmarkbage
sebmarkbage committed Nov 23, 2016
1 parent 9ceed8d commit 9773577
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 125 deletions.
2 changes: 0 additions & 2 deletions scripts/fiber/tests-failing.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions scripts/fiber/tests-passing.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
73 changes: 33 additions & 40 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(<div style={styles} />, container);

Expand All @@ -88,33 +106,31 @@ describe('ReactDOMComponent', () => {
styles.display = 'block';

ReactDOM.render(<div style={styles} />, 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(<div style={styles} />, 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(<div style={styles} />, 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(<div style={undefined} />, container);
expect(stubStyle.display).toBe('');
expect(stubStyle.fontFamily).toBe('');
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 {
Expand All @@ -125,31 +141,8 @@ describe('ReactDOMComponent', () => {
}
}

var stub = ReactTestUtils.renderIntoDocument(<App />);
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(<App />);
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(<span style={style} />, div);
style.background = 'blue';
ReactDOM.render(<span style={style} />, div);
expectDev(console.error.calls.count()).toBe(2);
ReactTestUtils.renderIntoDocument(<App />);
expectDev(() => style.position = 'absolute').toThrow();
});

it('should warn for unknown prop', () => {
Expand Down
87 changes: 4 additions & 83 deletions src/renderers/dom/stack/client/ReactDOMComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -763,10 +697,8 @@ ReactDOMComponent.Mixin = {
if (propKey === STYLE) {
if (propValue) {
if (__DEV__) {
// See `_updateDOMProperties`. style block
this._previousStyle = propValue;
Object.freeze(propValue);

This comment has been minimized.

Sign in to view

Copy link
@bvaughn

bvaughn Jan 27, 2017 •
edited
Loading

Contributor

Hey @sebmarkbage,

I just noticed this change. It causes a runtime error in react-virtualized because I'm temporarily caching style objects in Grid (eg when scrolling) to avoid tripping up the shallowCompare check. (I manage this caching to prevent user-provided style overrides from recreating style objects on each render.)

Unfortunately, certain components (like this) don't take into account that another component (this) is sometimes caching styles. And now that these cached styles are also being frozen, the first component may attempt beaks things when it tries to override a style.

I understand why we want to prevent style object mutation. It surprised me to learn that an object I created (in react-virtualized) was being frozen inside of React though. I wonder if others may have similar issues? Probably not too common a use-case I guess.

Edit: I'll fix this on the react-virtualized side.

This comment has been minimized.

Sign in to view

Copy link
@gaearon

gaearon Jan 27, 2017

Collaborator

I think this change was deemed to be safe because it would have triggered checkAndWarnForMutatedStyle warning as a deprecated pattern. Did it not?

This comment has been minimized.

Sign in to view

Copy link
@bvaughn

bvaughn Jan 27, 2017

Contributor

It didn't in this case, because my lower-level renderer was consistently overriding the style prop (so not really mutating the style) but now it trips up the read-only setter.

Seb and I chatted about this offline though and I think this is probably a reasonable change on React's side. I will just update RV to account for it.
}
propValue = this._previousStyleCopy = Object.assign({}, props.style);
}
propValue = CSSPropertyOperations.createMarkupForStyles(propValue, this);
}
Expand Down Expand Up @@ -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)) {
Expand All @@ -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) {
Expand All @@ -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`.
Expand Down

0 comments on commit 9773577

Please sign in to comment.