Skip to content

Commit

Permalink
changed ReactChildrenMutationWarningHook to Object.freeze
Browse files Browse the repository at this point in the history
  • Loading branch information
keyz committed Aug 30, 2016
1 parent f7076b7 commit df377b0
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 120 deletions.
29 changes: 19 additions & 10 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 @@ -231,13 +223,30 @@ ReactElement.createElement = function(type, config, children) {
// Children can be more than one argument, and those are transferred onto
// the newly allocated props object.
var childrenLength = arguments.length - 2;
var childArray;
if (childrenLength === 1) {
props.children = children;
if (Array.isArray(children)) {
childArray = children.slice(0);
// `slice` makes sure we don't freeze the original array reference
if (__DEV__) {
if (Object.freeze) {
Object.freeze(childArray);
}
}
props.children = childArray;
} else {
props.children = children;
}
} else if (childrenLength > 1) {
var childArray = Array(childrenLength);
childArray = Array(childrenLength);
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
30 changes: 4 additions & 26 deletions src/renderers/dom/server/__tests__/ReactServerRendering-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -539,37 +539,15 @@ describe('ReactServerRendering', function() {
expect(markup).toBe('<div></div>');
});

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

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() {
function normalizeCodeLocInfo(str) {
return str.replace(/\(at .+?:\d+\)/g, '(at **)');
}

it('should warn when children are mutated during render', function() {
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>{children}</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 @@ -432,7 +431,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', function() {
beforeEach(function() {
React = require('React');
Expand Down Expand Up @@ -49,30 +45,36 @@ describe('ReactComponent', function() {
}).toThrow();
});

it('should warn when children are mutated before render', function() {
it('should warn when children are mutated during render', function() {
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 **)'
);
function Wrapper(props) {
props.children[1] = <p key={1} />; // Mutation is illegal
return <div>{props.children}</div>;
}
expect(() => {
ReactTestUtils.renderIntoDocument(<Wrapper>{children}</Wrapper>);
}).toThrowError(/Cannot assign to read only property.*/);
});

it('should warn when children are mutated', function() {
it('should warn when children are mutated during update', function() {
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>;

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>;
}
}
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>{children}</Wrapper>);
}).toThrowError(/Cannot assign to read only property.*/);
});

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

0 comments on commit df377b0

Please sign in to comment.