Skip to content

Commit

Permalink
Fix memory leak in ReactChildrenMutationWarningHook for SSR (#7410)
Browse files Browse the repository at this point in the history
* corrected ReactChildrenMutationWarningHook's name

* changed `onComponentHasMounted` to `onMountComponent`

and get element from `ReactComponentTreeHook` instead of keeping an internal store
  • Loading branch information
keyz authored and gaearon committed Aug 10, 2016
1 parent c848b65 commit 5514ea3
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 15 deletions.
34 changes: 34 additions & 0 deletions src/renderers/dom/server/__tests__/ReactServerRendering-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -538,4 +538,38 @@ describe('ReactServerRendering', function() {
var markup = ReactServerRendering.renderToStaticMarkup(<Baz />);
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 **)');
}

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 **)'
);
});
});
20 changes: 5 additions & 15 deletions src/renderers/shared/hooks/ReactChildrenMutationWarningHook.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ var ReactComponentTreeHook = require('ReactComponentTreeHook');

var warning = require('warning');

var elements = {};

function handleElement(debugID, element) {
if (element == null) {
return;
Expand Down Expand Up @@ -46,21 +44,13 @@ function handleElement(debugID, element) {
);
}

var ReactDOMUnknownPropertyHook = {
onBeforeMountComponent(debugID, element) {
elements[debugID] = element;
},
onBeforeUpdateComponent(debugID, element) {
elements[debugID] = element;
},
onComponentHasMounted(debugID) {
handleElement(debugID, elements[debugID]);
delete elements[debugID];
var ReactChildrenMutationWarningHook = {
onMountComponent(debugID) {
handleElement(debugID, ReactComponentTreeHook.getElement(debugID));
},
onComponentHasUpdated(debugID) {
handleElement(debugID, elements[debugID]);
delete elements[debugID];
handleElement(debugID, ReactComponentTreeHook.getElement(debugID));
},
};

module.exports = ReactDOMUnknownPropertyHook;
module.exports = ReactChildrenMutationWarningHook;

0 comments on commit 5514ea3

Please sign in to comment.