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.

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

Jump to bottom

Add warning for unknown properties. #6800

Merged
merged 1 commit into from
May 25, 2016
Merged

Add warning for unknown properties. #6800

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
14 changes: 13 additions & 1 deletion src/addons/transitions/ReactTransitionGroup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,21 @@ var ReactTransitionGroup = React.createClass({
));
}
}

// Do not forward ReactTransitionGroup props to primitive DOM nodes
var props = Object.assign({}, this.props);
delete props.transitionLeave;
delete props.transitionName;
delete props.transitionAppear;
delete props.transitionEnter;
delete props.childFactory;
delete props.transitionLeaveTimeout;
delete props.transitionEnterTimeout;
delete props.component;

return React.createElement(
this.props.component,
this.props,
props,
childrenToRender
);
},
Expand Down
40 changes: 30 additions & 10 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ describe('ReactDOMComponent', function() {
var ReactDOMServer;
var inputValueTracking;

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

beforeEach(function() {
jest.resetModuleRegistry();
React = require('React');
Expand Down Expand Up @@ -148,6 +152,17 @@ describe('ReactDOMComponent', function() {
expect(console.error.argsForCall.length).toBe(2);
});

it('should warn for unknown prop', function() {
spyOn(console, 'error');
var container = document.createElement('div');
ReactDOM.render(<div foo="bar" />, container);
expect(console.error.argsForCall.length).toBe(1);
expect(normalizeCodeLocInfo(console.error.argsForCall[0][0])).toBe(
'Warning: Unknown prop `foo` on <div> tag. Remove this prop from the element. ' +
'For details, see https://fb.me/react-unknown-prop\n in div (at **)'
);
});

it('should warn about styles with numeric string values for non-unitless properties', function() {
spyOn(console, 'error');

Expand Down Expand Up @@ -1332,15 +1347,15 @@ describe('ReactDOMComponent', function() {
ReactDOMServer.renderToString(<input type="text" onclick="1"/>);
expect(console.error.argsForCall.length).toBe(2);
expect(
console.error.argsForCall[0][0].replace(/\(.+?:\d+\)/g, '(**:*)')
normalizeCodeLocInfo(console.error.argsForCall[0][0])
).toBe(
'Warning: Unknown DOM property class. Did you mean className? (**:*)'
'Warning: Unknown DOM property class. Did you mean className?\n in div (at **)'
);
expect(
console.error.argsForCall[1][0].replace(/\(.+?:\d+\)/g, '(**:*)')
normalizeCodeLocInfo(console.error.argsForCall[1][0])
).toBe(
'Warning: Unknown event handler property onclick. Did you mean ' +
'`onClick`? (**:*)'
'`onClick`?\n in input (at **)'
);
});

Expand All @@ -1354,10 +1369,11 @@ describe('ReactDOMComponent', function() {
ReactDOMServer.renderToString(<div class="paladin" />, container);
expect(console.error.argsForCall.length).toBe(1);
expect(
console.error.argsForCall[0][0].replace(/\(.+?:\d+\)/g, '(**:*)')
normalizeCodeLocInfo(console.error.argsForCall[0][0])
).toBe(
'Warning: Unknown DOM property class. Did you mean className? (**:*)'
'Warning: Unknown DOM property class. Did you mean className?\n in div (at **)'
);

});

it('gives source code refs for unknown prop warning for exact elements ', function() {
Expand All @@ -1375,10 +1391,12 @@ describe('ReactDOMComponent', function() {

expect(console.error.argsForCall.length).toBe(2);

var matches = console.error.argsForCall[0][0].match(/.*className.*\(.*:(\d+)\)/);
expect(console.error.argsForCall[0][0]).toContain('className');
var matches = console.error.argsForCall[0][0].match(/.*\(.*:(\d+)\).*/);
var previousLine = matches[1];

matches = console.error.argsForCall[1][0].match(/.*onClick.*\(.*:(\d+)\)/);
expect(console.error.argsForCall[1][0]).toContain('onClick');
matches = console.error.argsForCall[1][0].match(/.*\(.*:(\d+)\).*/);
var currentLine = matches[1];

//verify line number has a proper relative difference,
Expand Down Expand Up @@ -1424,10 +1442,12 @@ describe('ReactDOMComponent', function() {

expect(console.error.argsForCall.length).toBe(2);

var matches = console.error.argsForCall[0][0].match(/.*className.*\(.*:(\d+)\)/);
expect(console.error.argsForCall[0][0]).toContain('className');
var matches = console.error.argsForCall[0][0].match(/.*\(.*:(\d+)\).*/);
var previousLine = matches[1];

matches = console.error.argsForCall[1][0].match(/.*onClick.*\(.*:(\d+)\)/);
expect(console.error.argsForCall[1][0]).toContain('onClick');
matches = console.error.argsForCall[1][0].match(/.*\(.*:(\d+)\).*/);
var currentLine = matches[1];

//verify line number has a proper relative difference,
Expand Down
74 changes: 47 additions & 27 deletions src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

var DOMProperty = require('DOMProperty');
var EventPluginRegistry = require('EventPluginRegistry');
var ReactComponentTreeDevtool = require('ReactComponentTreeDevtool');

var warning = require('warning');

Expand All @@ -22,10 +23,19 @@ if (__DEV__) {
dangerouslySetInnerHTML: true,
key: true,
ref: true,

defaultValue: true,
valueLink: true,
defaultChecked: true,
checkedLink: true,
innerHTML: true,
suppressContentEditableWarning: true,
onFocusIn: true,
onFocusOut: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suppressContentEditableWarning seems okay, but what is innerHTML here for? and onFocusIn and onFocusOut? I also don't love that the form props are here but I guess it's not terrible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

innerHTML, onFocusIn, and onFocusOut already emit warnings telling the user not to use them. Having it also emit this warning is just confusing, so I blacklisted those tags that we already warn about.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimfb can you followup and add a comment in the code about this? This inline discussion will be lost in the sands of time.

};
var warnedProperties = {};

var warnUnknownProperty = function(name, source) {
var warnUnknownProperty = function(tagName, name, debugID) {
if (DOMProperty.properties.hasOwnProperty(name) || DOMProperty.isCustomAttribute(name)) {
return;
}
Expand All @@ -48,16 +58,6 @@ if (__DEV__) {
null
);

// For now, only warn when we have a suggested correction. This prevents
// logging too much when using transferPropsTo.
warning(
standardName == null,
'Unknown DOM property %s. Did you mean %s? %s',
name,
standardName,
formatSource(source)
);

var registrationName = (
EventPluginRegistry.possibleRegistrationNames.hasOwnProperty(
lowerCasedName
Expand All @@ -66,36 +66,56 @@ if (__DEV__) {
null
);

warning(
registrationName == null,
'Unknown event handler property %s. Did you mean `%s`? %s',
name,
registrationName,
formatSource(source)
);
};

var formatSource = function(source) {
return source ? `(${source.fileName.replace(/^.*[\\\/]/, '')}:${source.lineNumber})` : '';
if (standardName != null) {
warning(
standardName == null,
'Unknown DOM property %s. Did you mean %s?%s',
name,
standardName,
ReactComponentTreeDevtool.getStackAddendumByID(debugID)
);
} else if (registrationName != null) {
warning(
registrationName == null,
'Unknown event handler property %s. Did you mean `%s`?%s',
name,
registrationName,
ReactComponentTreeDevtool.getStackAddendumByID(debugID)
);
} else {
// We were unable to guess which prop the user intended.
// It is likely that the user was just blindly spreading/forwarding props
// Components should be careful to only render valid props/attributes.
warning(
false,
'Unknown prop `%s` on <%s> tag. Remove this prop from the element. ' +
'For details, see https://fb.me/react-unknown-prop%s',
name,
tagName,
ReactComponentTreeDevtool.getStackAddendumByID(debugID)
);
}
};

}

function handleElement(element) {
function handleElement(debugID, element) {
if (element == null || typeof element.type !== 'string') {
return;
}
if (element.type.indexOf('-') >= 0 || element.props.is) {
return;
}
for (var key in element.props) {
warnUnknownProperty(key, element._source);
warnUnknownProperty(element.type, key, debugID);
}
}

var ReactDOMUnknownPropertyDevtool = {
onBeforeMountComponent(debugID, element) {
handleElement(element);
handleElement(debugID, element);
},
onBeforeUpdateComponent(debugID, element) {
handleElement(element);
handleElement(debugID, element);
},
};

Expand Down