Skip to content

Commit

Permalink
changed ReactChildrenMutationWarningHook to Object.freeze (#7455)
Browse files Browse the repository at this point in the history
- only freeze children array created by createElement
  • Loading branch information
keyz authored and gaearon committed Sep 13, 2016
1 parent 616e468 commit 38c4ade
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 121 deletions.
13 changes: 5 additions & 8 deletions src/isomorphic/classic/element/ReactElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ var ReactElement = function(type, key, ref, self, source, owner, props) {
// This can be replaced with a WeakMap once they are implemented in
// commonly used development environments.
element._store = {};
var shadowChildren = Array.isArray(props.children) ? props.children.slice(0) : props.children;

// To make comparing ReactElements easier for testing purposes, we make
// the validation flag non-enumerable (where possible, which should
Expand All @@ -155,12 +154,6 @@ var ReactElement = function(type, key, ref, self, source, owner, props) {
writable: false,
value: self,
});
Object.defineProperty(element, '_shadowChildren', {
configurable: false,
enumerable: false,
writable: false,
value: shadowChildren,
});
// Two elements created in two different places should be considered
// equal for testing purposes and therefore we hide it from enumeration.
Object.defineProperty(element, '_source', {
Expand All @@ -172,7 +165,6 @@ var ReactElement = function(type, key, ref, self, source, owner, props) {
} else {
element._store.validated = false;
element._self = self;
element._shadowChildren = shadowChildren;
element._source = source;
}
if (Object.freeze) {
Expand Down Expand Up @@ -238,6 +230,11 @@ ReactElement.createElement = function(type, config, children) {
for (var i = 0; i < childrenLength; i++) {
childArray[i] = arguments[i + 2];
}
if (__DEV__) {
if (Object.freeze) {
Object.freeze(childArray);
}
}
props.children = childArray;
}

Expand Down
37 changes: 10 additions & 27 deletions src/renderers/dom/server/__tests__/ReactServerRendering-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -539,37 +539,20 @@ describe('ReactServerRendering', () => {
expect(markup).toBe('<div></div>');
});

it('warns when children are mutated before render', () => {
function normalizeCodeLocInfo(str) {
return str.replace(/\(at .+?:\d+\)/g, '(at **)');
}

it('should warn when children are mutated during render', () => {
spyOn(console, 'error');
var children = [<span key={0} />, <span key={1} />, <span key={2} />];
var element = <div>{children}</div>;
children[1] = <p key={1} />; // Mutation is illegal
ReactServerRendering.renderToString(element);
expect(console.error.calls.count()).toBe(1);
expect(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: Component\'s children should not be mutated.\n in div (at **)'
);
});

it('should warn when children are mutated', () => {
function normalizeCodeLocInfo(str) {
return str.replace(/\(at .+?:\d+\)/g, '(at **)');
}

spyOn(console, 'error');
var children = [<span key={0} />, <span key={1} />, <span key={2} />];
function Wrapper(props) {
props.children[1] = <p key={1} />; // Mutation is illegal
return <div>{props.children}</div>;
}
ReactServerRendering.renderToString(<Wrapper>{children}</Wrapper>);
expect(console.error.calls.count()).toBe(1);
expect(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: Component\'s children should not be mutated.\n in Wrapper (at **)'
);
expect(() => {
ReactServerRendering.renderToStaticMarkup(
<Wrapper>
<span key={0}/>
<span key={1}/>
<span key={2}/>
</Wrapper>
);
}).toThrowError(/Cannot assign to read only property.*/);
});
});
2 changes: 0 additions & 2 deletions src/renderers/shared/ReactDebugTool.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
var ReactInvalidSetStateWarningHook = require('ReactInvalidSetStateWarningHook');
var ReactHostOperationHistoryHook = require('ReactHostOperationHistoryHook');
var ReactComponentTreeHook = require('ReactComponentTreeHook');
var ReactChildrenMutationWarningHook = require('ReactChildrenMutationWarningHook');
var ExecutionEnvironment = require('ExecutionEnvironment');

var performanceNow = require('performanceNow');
Expand Down Expand Up @@ -417,7 +416,6 @@ var ReactDebugTool = {

ReactDebugTool.addHook(ReactInvalidSetStateWarningHook);
ReactDebugTool.addHook(ReactComponentTreeHook);
ReactDebugTool.addHook(ReactChildrenMutationWarningHook);
var url = (ExecutionEnvironment.canUseDOM && window.location.href) || '';
if ((/[?&]react_perf\b/).test(url)) {
ReactDebugTool.beginProfiling();
Expand Down
61 changes: 0 additions & 61 deletions src/renderers/shared/hooks/ReactChildrenMutationWarningHook.js

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@ var React;
var ReactDOM;
var ReactTestUtils;

function normalizeCodeLocInfo(str) {
return str.replace(/\(at .+?:\d+\)/g, '(at **)');
}

describe('ReactComponent', () => {
beforeEach(() => {
React = require('React');
Expand Down Expand Up @@ -49,30 +45,46 @@ describe('ReactComponent', () => {
}).toThrow();
});

it('should warn when children are mutated before render', () => {
spyOn(console, 'error');
var children = [<span key={0} />, <span key={1} />, <span key={2} />];
var element = <div>{children}</div>;
children[1] = <p key={1} />; // Mutation is illegal
ReactTestUtils.renderIntoDocument(element);
expect(console.error.calls.count()).toBe(1);
expect(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: Component\'s children should not be mutated.\n in div (at **)'
);
});

it('should warn when children are mutated', () => {
it('should warn when children are mutated during render', () => {
spyOn(console, 'error');
var children = [<span key={0} />, <span key={1} />, <span key={2} />];
function Wrapper(props) {
props.children[1] = <p key={1} />; // Mutation is illegal
return <div>{props.children}</div>;
}
ReactTestUtils.renderIntoDocument(<Wrapper>{children}</Wrapper>);
expect(console.error.calls.count()).toBe(1);
expect(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: Component\'s children should not be mutated.\n in Wrapper (at **)'
);
expect(() => {
ReactTestUtils.renderIntoDocument(
<Wrapper>
<span key={0}/>
<span key={1}/>
<span key={2}/>
</Wrapper>
);
}).toThrowError(/Cannot assign to read only property.*/);
});

it('should warn when children are mutated during update', () => {
spyOn(console, 'error');

class Wrapper extends React.Component {
componentDidMount() {
this.props.children[1] = <p key={1} />; // Mutation is illegal
this.forceUpdate();
}

render() {
return <div>{this.props.children}</div>;
}
}

expect(() => {
ReactTestUtils.renderIntoDocument(
<Wrapper>
<span key={0}/>
<span key={1}/>
<span key={2}/>
</Wrapper>
);
}).toThrowError(/Cannot assign to read only property.*/);
});

it('should support refs on owned components', () => {
Expand Down

0 comments on commit 38c4ade

Please sign in to comment.