Skip to content

Freeze ReactElement.props in dev mode #4172

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 19, 2015
Merged

Freeze ReactElement.props in dev mode #4172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 8 additions & 76 deletions src/isomorphic/classic/element/ReactElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,71 +14,12 @@
var ReactCurrentOwner = require('ReactCurrentOwner');

var assign = require('Object.assign');
var warning = require('warning');

var RESERVED_PROPS = {
key: true,
ref: true,
};

/**
* Warn for mutations.
*
* @internal
* @param {object} object
* @param {string} key
*/
function defineWarningProperty(object, key) {
Object.defineProperty(object, key, {

configurable: false,
enumerable: true,

get: function() {
if (!this._store) {
return null;
}
return this._store[key];
},

set: function(value) {
warning(
false,
'Don\'t set the %s property of the React element. Instead, ' +
'specify the correct value when initially creating the element.',
key
);
this._store[key] = value;
},

});
}

/**
* This is updated to true if the membrane is successfully created.
*/
var useMutationMembrane = false;

/**
* Warn for mutations.
*
* @internal
* @param {object} prototype
*/
function defineMutationMembrane(prototype) {
try {
var pseudoFrozenProperties = {
props: true,
};
for (var key in pseudoFrozenProperties) {
defineWarningProperty(prototype, key);
}
useMutationMembrane = true;
} catch (x) {
// IE will fail on defineProperty
}
}

/**
* Base constructor for all React elements. This is only used to make this
* work with a dynamic instanceof check. Nothing should live on this prototype.
Expand All @@ -100,11 +41,11 @@ var ReactElement = function(type, key, ref, owner, props) {
this._owner = owner;

if (__DEV__) {
// The validation flag and props are currently mutative. We put them on
// The validation flag is currently mutative. We put it on
// an external backing store so that we can freeze the whole object.
// This can be replaced with a WeakMap once they are implemented in
// commonly used development environments.
this._store = {props: props, originalProps: assign({}, props)};
this._store = {};

// To make comparing ReactElements easier for testing purposes, we make
// the validation flag non-enumerable (where possible, which should
Expand All @@ -115,21 +56,15 @@ var ReactElement = function(type, key, ref, owner, props) {
configurable: false,
enumerable: false,
writable: true,
value: false,
});
} catch (x) {
this._store.validated = false;
}
this._store.validated = false;

// We're not allowed to set props directly on the object so we early
// return and rely on the prototype membrane to forward to the backing
// store.
if (useMutationMembrane) {
Object.freeze(this);
return;
}
this.props = props;
Object.freeze(this.props);
Object.freeze(this);
}

this.props = props;
};

// We intentionally don't expose the function on the constructor property.
Expand All @@ -138,10 +73,6 @@ ReactElement.prototype = {
_isReactElement: true,
};

if (__DEV__) {
defineMutationMembrane(ReactElement.prototype);
}

ReactElement.createElement = function(type, config, children) {
var propName;

Expand Down Expand Up @@ -219,6 +150,7 @@ ReactElement.cloneAndReplaceProps = function(oldElement, newProps) {
// If the key on the original is valid, then the clone is valid
newElement._store.validated = oldElement._store.validated;
}

return newElement;
};

Expand Down
86 changes: 0 additions & 86 deletions src/isomorphic/classic/element/ReactElementValidator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,90 +286,6 @@ function checkPropTypes(componentName, propTypes, props, location) {
}
}

var warnedPropsMutations = {};

/**
* Warn about mutating props when setting `propName` on `element`.
*
* @param {string} propName The string key within props that was set
* @param {ReactElement} element
*/
function warnForPropsMutation(propName, element) {
var type = element.type;
var elementName = typeof type === 'string' ? type : type.displayName;
var ownerName = element._owner ?
element._owner.getPublicInstance().constructor.displayName : null;

var warningKey = propName + '|' + elementName + '|' + ownerName;
if (warnedPropsMutations.hasOwnProperty(warningKey)) {
return;
}
warnedPropsMutations[warningKey] = true;

var elementInfo = '';
if (elementName) {
elementInfo = ' <' + elementName + ' />';
}
var ownerInfo = '';
if (ownerName) {
ownerInfo = ' The element was created by ' + ownerName + '.';
}

warning(
false,
'Don\'t set .props.%s of the React component%s. Instead, specify the ' +
'correct value when initially creating the element or use ' +
'React.cloneElement to make a new element with updated props.%s',
propName,
elementInfo,
ownerInfo
);
}

// Inline Object.is polyfill
function is(a, b) {
if (a !== a) {
// NaN
return b !== b;
}
if (a === 0 && b === 0) {
// +-0
return 1 / a === 1 / b;
}
return a === b;
}

/**
* Given an element, check if its props have been mutated since element
* creation (or the last call to this function). In particular, check if any
* new props have been added, which we can't directly catch by defining warning
* properties on the props object.
*
* @param {ReactElement} element
*/
function checkAndWarnForMutatedProps(element) {
if (!element._store) {
// Element was created using `new ReactElement` directly or with
// `ReactElement.createElement`; skip mutation checking
return;
}

var originalProps = element._store.originalProps;
var props = element.props;

for (var propName in props) {
if (props.hasOwnProperty(propName)) {
if (!originalProps.hasOwnProperty(propName) ||
!is(originalProps[propName], props[propName])) {
warnForPropsMutation(propName, element);

// Copy over the new value so that the two props objects match again
originalProps[propName] = props[propName];
}
}
}
}

/**
* Given an element, validate that its props follow the propTypes definition,
* provided by the type.
Expand Down Expand Up @@ -409,8 +325,6 @@ function validatePropTypes(element) {

var ReactElementValidator = {

checkAndWarnForMutatedProps: checkAndWarnForMutatedProps,

createElement: function(type, props, children) {
// We warn in this case but don't throw. We expect the element creation to
// succeed and there will likely be errors in render.
Expand Down
84 changes: 25 additions & 59 deletions src/isomorphic/classic/element/__tests__/ReactElement-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,19 @@ describe('ReactElement', function() {
expect(element.type).toBe(ComponentClass);
expect(element.key).toBe(null);
expect(element.ref).toBe(null);
expect(element.props).toEqual({});
var expectation = {};
Object.freeze(expectation);
expect(element.props).toEqual(expectation);
});

it('allows a string to be passed as the type', function() {
var element = React.createFactory('div')();
expect(element.type).toBe('div');
expect(element.key).toBe(null);
expect(element.ref).toBe(null);
expect(element.props).toEqual({});
var expectation = {};
Object.freeze(expectation);
expect(element.props).toEqual(expectation);
});

it('returns an immutable element', function() {
Expand All @@ -70,7 +74,9 @@ describe('ReactElement', function() {
expect(element.type).toBe(ComponentClass);
expect(element.key).toBe('12');
expect(element.ref).toBe('34');
expect(element.props).toEqual({foo:'56'});
var expectation = {foo:'56'};
Object.freeze(expectation);
expect(element.props).toEqual(expectation);
});

it('coerces the key to a string', function() {
Expand All @@ -81,7 +87,9 @@ describe('ReactElement', function() {
expect(element.type).toBe(ComponentClass);
expect(element.key).toBe('12');
expect(element.ref).toBe(null);
expect(element.props).toEqual({foo:'56'});
var expectation = {foo:'56'};
Object.freeze(expectation);
expect(element.props).toEqual(expectation);
});

it('preserves the owner on the element', function() {
Expand Down Expand Up @@ -243,84 +251,42 @@ describe('ReactElement', function() {
expect(inst2.props.prop).toBe(null);
});

it('warns when changing a prop after element creation', function() {
spyOn(console, 'error');
it('throws when changing a prop (in dev) after element creation', function() {
var Outer = React.createClass({
render: function() {
var el = <div className="moo" />;

// This assignment warns but should still work for now.
el.props.className = 'quack';
expect(el.props.className).toBe('quack');
expect(function() {
el.props.className = 'quack';
}).toThrow();
expect(el.props.className).toBe('moo');

return el;
},
});
var outer = ReactTestUtils.renderIntoDocument(<Outer color="orange" />);
expect(React.findDOMNode(outer).className).toBe('quack');

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain(
'Don\'t set .props.className of the React component <div />.'
);
expect(console.error.argsForCall[0][0]).toContain(
'The element was created by Outer.'
);

console.error.reset();

// This also warns (just once per key/type pair)
outer.props.color = 'green';
outer.forceUpdate();
outer.props.color = 'purple';
outer.forceUpdate();

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain(
'Don\'t set .props.color of the React component <Outer />.'
);
expect(React.findDOMNode(outer).className).toBe('moo');
});

it('warns when adding a prop after element creation', function() {
spyOn(console, 'error');
it('throws when adding a prop (in dev) after element creation', function() {
var container = document.createElement('div');
var Outer = React.createClass({
getDefaultProps: () => ({sound: 'meow'}),
render: function() {
var el = <div>{this.props.sound}</div>;

// This assignment doesn't warn immediately (because we can't) but it
// warns upon mount.
el.props.className = 'quack';
expect(el.props.className).toBe('quack');
expect(function() {
el.props.className = 'quack';
}).toThrow();

expect(el.props.className).toBe(undefined);

return el;
},
});
var outer = React.render(<Outer />, container);
expect(React.findDOMNode(outer).textContent).toBe('meow');
expect(React.findDOMNode(outer).className).toBe('quack');

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain(
'Don\'t set .props.className of the React component <div />.'
);
expect(console.error.argsForCall[0][0]).toContain(
'The element was created by Outer.'
);

console.error.reset();

var newOuterEl = <Outer />;
newOuterEl.props.sound = 'oink';
outer = React.render(newOuterEl, container);
expect(React.findDOMNode(outer).textContent).toBe('oink');
expect(React.findDOMNode(outer).className).toBe('quack');

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain(
'Don\'t set .props.sound of the React component <Outer />.'
);
expect(React.findDOMNode(outer).className).toBe('');
});

it('does not warn for NaN props', function() {
Expand Down
12 changes: 8 additions & 4 deletions src/isomorphic/modern/class/__tests__/ReactES6Class-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ var React;
describe('ReactES6Class', function() {

var container;
var freeze = function(expectation) {
Object.freeze(expectation);
return expectation;
};
var Inner;
var attachedListener = null;
var renderedName = null;
Expand Down Expand Up @@ -288,10 +292,10 @@ describe('ReactES6Class', function() {
lifeCycles = []; // reset
test(<Foo value="bar" />, 'SPAN', 'bar');
expect(lifeCycles).toEqual([
'receive-props', {value: 'bar'},
'should-update', {value: 'bar'}, {},
'will-update', {value: 'bar'}, {},
'did-update', {value: 'foo'}, {},
'receive-props', freeze({value: 'bar'}),
'should-update', freeze({value: 'bar'}), {},
'will-update', freeze({value: 'bar'}), {},
'did-update', freeze({value: 'foo'}), {},
]);
lifeCycles = []; // reset
React.unmountComponentAtNode(container);
Expand Down
Loading