Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error for style mutation #4946

Merged
merged 1 commit into from
Sep 22, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 28 additions & 2 deletions src/renderers/dom/shared/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,32 @@ function legacyReplaceProps(partialProps, callback) {
}
}

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 becauses undefined and that
// inf and nan don't become null
return String(obj);
}

var styleMutationWarning = {};

function checkAndWarnForMutatedStyle(style1, style2, component) {
Expand Down Expand Up @@ -202,8 +228,8 @@ function checkAndWarnForMutatedStyle(style1, style2, component) {
'the `render` %s. Previous style: %s. Mutated style: %s.',
componentName,
owner ? 'of `' + ownerName + '`' : 'using <' + componentName + '>',
JSON.stringify(style1),
JSON.stringify(style2)
friendlyStringify(style1),
friendlyStringify(style2)
);
}

Expand Down
21 changes: 19 additions & 2 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,8 @@ describe('ReactDOMComponent', function() {
'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"}.'
'{border: "1px solid black"}. Mutated style: ' +
'{border: "1px solid black", position: "absolute"}.'
);

style = {background: 'red'};
Expand All @@ -147,6 +147,23 @@ describe('ReactDOMComponent', function() {
expect(console.error.argsForCall.length).toBe(2);
});

it('should warn semi-nicely about NaN in style', function() {
spyOn(console, 'error');

var style = {fontSize: NaN};
var div = document.createElement('div');
ReactDOM.render(<span style={style}></span>, div);
ReactDOM.render(<span style={style}></span>, div);

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toEqual(
'Warning: `span` was passed a style object that has previously been ' +
'mutated. Mutating `style` is deprecated. Consider cloning it ' +
'beforehand. Check the `render` using <span>. Previous style: ' +
'{fontSize: NaN}. Mutated style: {fontSize: NaN}.'
);
});

it('should update styles if initially null', function() {
var styles = null;
var container = document.createElement('div');
Expand Down