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 hasOwnProperty checks where appropriate #1189

Closed
wants to merge 1 commit into from
Closed

Add hasOwnProperty checks where appropriate #1189

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/addons/transitions/ReactTransitionChildMapping.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ var ReactTransitionChildMapping = {

var pendingKeys = [];
for (var prevKey in prev) {
if (next[prevKey]) {
if (next.hasOwnProperty(prevKey)) {
if (pendingKeys.length) {
nextKeysPending[prevKey] = pendingKeys;
pendingKeys = [];
Expand All @@ -83,7 +83,7 @@ var ReactTransitionChildMapping = {
var i;
var childMapping = {};
for (var nextKey in next) {
if (nextKeysPending[nextKey]) {
if (nextKeysPending.hasOwnProperty(nextKey)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should some of these hasOwnProperty checks inside for-in loops be early continue instead of wrapping in a conditional? Now it is quite easy to still run some code after the conditional but still inside the for in loop, perhaps unintentionally? But I don't know those few places in this diff so well.

for (i = 0; i < nextKeysPending[nextKey].length; i++) {
var pendingNextKey = nextKeysPending[nextKey][i];
childMapping[nextKeysPending[nextKey][i]] = getValueForKey(
Expand Down
2 changes: 1 addition & 1 deletion src/addons/update.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ function update(value, spec) {
}

for (var k in spec) {
if (!ALL_DIRECTIVES_SET[k]) {
if (!(ALL_DIRECTIVES_SET.hasOwnProperty(k) && ALL_DIRECTIVES_SET[k])) {
nextValue[k] = update(value[k], spec[k]);
}
}
Expand Down
9 changes: 6 additions & 3 deletions src/browser/ReactEventEmitter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ var topEventMapping = {
var topListenersIDKey = "_reactListenersID" + String(Math.random()).slice(2);

function getListeningForDocument(mountAt) {
if (mountAt[topListenersIDKey] == null) {
if (!mountAt.hasOwnProperty(topListenersIDKey)) {
Copy link
Member

Choose a reason for hiding this comment

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

From @salier - "This is breaking in IE8, where mountAt is [object DispHTMLDocument]."

We can probably just revert this part.

mountAt[topListenersIDKey] = reactTopListenersCounter++;
alreadyListeningTo[mountAt[topListenersIDKey]] = {};
}
Expand Down Expand Up @@ -253,7 +253,10 @@ var ReactEventEmitter = merge(ReactEventEmitterMixin, {
var topLevelTypes = EventConstants.topLevelTypes;
for (var i = 0, l = dependencies.length; i < l; i++) {
var dependency = dependencies[i];
if (!isListening[dependency]) {
if (!(
isListening.hasOwnProperty(dependency) &&
isListening[dependency]
)) {
var topLevelType = topLevelTypes[dependency];

if (topLevelType === topLevelTypes.topWheel) {
Expand Down Expand Up @@ -292,7 +295,7 @@ var ReactEventEmitter = merge(ReactEventEmitterMixin, {
// to make sure blur and focus event listeners are only attached once
isListening[topLevelTypes.topBlur] = true;
isListening[topLevelTypes.topFocus] = true;
} else if (topEventMapping[dependency]) {
} else if (topEventMapping.hasOwnProperty(dependency)) {
trapBubbledEvent(topLevelType, topEventMapping[dependency], mountAt);
}

Expand Down
13 changes: 6 additions & 7 deletions src/browser/eventPlugins/AnalyticsEventPluginFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,19 +149,18 @@ function extractEvents(
topLevelTargetID,
nativeEvent) {
var currentEvent = topLevelTypesToAnalyticsEvent[topLevelType];
if (!currentEvent || !topLevelTarget || !topLevelTarget.attributes) {
if (!currentEvent || !topLevelTarget) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm looking at another comment where this broken something... investigating

return null;
}

var analyticsIDAttribute = topLevelTarget.attributes[ANALYTICS_ID];
var analyticsEventsAttribute = topLevelTarget.attributes[ANALYTICS_EVENTS];
if(!analyticsIDAttribute || !analyticsEventsAttribute) {
var analyticsID = topLevelTarget.getAttribute(ANALYTICS_ID);
var analyticsEventsStr = topLevelTarget.getAttribute(ANALYTICS_EVENTS);
if (!analyticsID || !analyticsEventsStr) {
return null;
}

var analyticsEventsArr = analyticsEventsAttribute.value.split(",");
var analyticsID = analyticsIDAttribute.value;
if (!analyticsData[analyticsID]) {
var analyticsEventsArr = analyticsEventsStr.split(",");
if (!analyticsData.hasOwnProperty(analyticsID)) {
initAnalyticsDataForID(analyticsID, analyticsEventsArr);
}

Expand Down
6 changes: 3 additions & 3 deletions src/browser/ui/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ ReactDOMComponent.Mixin = {
if (propValue == null) {
continue;
}
if (registrationNameModules[propKey]) {
if (registrationNameModules.hasOwnProperty(propKey)) {
putListener(this._rootNodeID, propKey, propValue, transaction);
} else {
if (propKey === STYLE) {
Expand Down Expand Up @@ -271,7 +271,7 @@ ReactDOMComponent.Mixin = {
styleUpdates[styleName] = '';
}
}
} else if (registrationNameModules[propKey]) {
} else if (registrationNameModules.hasOwnProperty(propKey)) {
deleteListener(this._rootNodeID, propKey);
} else if (
DOMProperty.isStandardName[propKey] ||
Expand Down Expand Up @@ -313,7 +313,7 @@ ReactDOMComponent.Mixin = {
// Relies on `updateStylesByID` not mutating `styleUpdates`.
styleUpdates = nextProp;
}
} else if (registrationNameModules[propKey]) {
} else if (registrationNameModules.hasOwnProperty(propKey)) {
putListener(this._rootNodeID, propKey, nextProp, transaction);
} else if (
DOMProperty.isStandardName[propKey] ||
Expand Down
24 changes: 14 additions & 10 deletions src/browser/ui/dom/DOMProperty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ var DOMPropertyInjection = {

for (var propName in Properties) {
invariant(
!DOMProperty.isStandardName[propName],
!DOMProperty.isStandardName.hasOwnProperty(propName),
'injectDOMPropertyConfig(...): You\'re trying to inject DOM property ' +
'\'%s\' which has already been injected. You may be accidentally ' +
'injecting the same DOM property config twice, or you may be ' +
Expand All @@ -86,19 +86,23 @@ var DOMPropertyInjection = {
var lowerCased = propName.toLowerCase();
DOMProperty.getPossibleStandardName[lowerCased] = propName;

var attributeName = DOMAttributeNames[propName];
if (attributeName) {
if (DOMAttributeNames.hasOwnProperty(propName)) {
var attributeName = DOMAttributeNames[propName];
DOMProperty.getPossibleStandardName[attributeName] = propName;
DOMProperty.getAttributeName[propName] = attributeName;
} else {
DOMProperty.getAttributeName[propName] = lowerCased;
}

DOMProperty.getAttributeName[propName] = attributeName || lowerCased;

DOMProperty.getPropertyName[propName] =
DOMPropertyNames[propName] || propName;

var mutationMethod = DOMMutationMethods[propName];
if (mutationMethod) {
DOMProperty.getMutationMethod[propName] = mutationMethod;
DOMPropertyNames.hasOwnProperty(propName) ?
DOMPropertyNames[propName] :
propName;

if (DOMMutationMethods.hasOwnProperty(propName)) {
DOMProperty.getMutationMethod[propName] = DOMMutationMethods[propName];
} else {
DOMProperty.getMutationMethod[propName] = null;
}

var propConfig = Properties[propName];
Expand Down
21 changes: 15 additions & 6 deletions src/browser/ui/dom/DOMPropertyOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,22 @@ if (__DEV__) {
var warnedProperties = {};

var warnUnknownProperty = function(name) {
if (reactProps[name] || warnedProperties[name]) {
if (reactProps.hasOwnProperty(name) && reactProps[name] ||
warnedProperties.hasOwnProperty(name) && warnedProperties[name]) {
return;
}

warnedProperties[name] = true;
var lowerCasedName = name.toLowerCase();

// data-* attributes should be lowercase; suggest the lowercase version
var standardName = DOMProperty.isCustomAttribute(lowerCasedName) ?
lowerCasedName : DOMProperty.getPossibleStandardName[lowerCasedName];
var standardName = (
DOMProperty.isCustomAttribute(lowerCasedName) ?
lowerCasedName :
DOMProperty.getPossibleStandardName.hasOwnProperty(lowerCasedName) ?
DOMProperty.getPossibleStandardName[lowerCasedName] :
null
);

// For now, only warn when we have a suggested correction. This prevents
// logging too much when using transferPropsTo.
Expand Down Expand Up @@ -90,7 +96,8 @@ var DOMPropertyOperations = {
* @return {?string} Markup string, or null if the property was invalid.
*/
createMarkupForProperty: function(name, value) {
if (DOMProperty.isStandardName[name]) {
if (DOMProperty.isStandardName.hasOwnProperty(name) &&
DOMProperty.isStandardName[name]) {
if (shouldIgnoreValue(name, value)) {
return '';
}
Expand Down Expand Up @@ -120,7 +127,8 @@ var DOMPropertyOperations = {
* @param {*} value
*/
setValueForProperty: function(node, name, value) {
if (DOMProperty.isStandardName[name]) {
if (DOMProperty.isStandardName.hasOwnProperty(name) &&
DOMProperty.isStandardName[name]) {
var mutationMethod = DOMProperty.getMutationMethod[name];
if (mutationMethod) {
mutationMethod(node, value);
Expand Down Expand Up @@ -152,7 +160,8 @@ var DOMPropertyOperations = {
* @param {string} name
*/
deleteValueForProperty: function(node, name) {
if (DOMProperty.isStandardName[name]) {
if (DOMProperty.isStandardName.hasOwnProperty(name) &&
DOMProperty.isStandardName[name]) {
var mutationMethod = DOMProperty.getMutationMethod[name];
if (mutationMethod) {
mutationMethod(node, undefined);
Expand Down
13 changes: 8 additions & 5 deletions src/browser/ui/dom/dangerousStyleValue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,18 @@

var CSSProperty = require('CSSProperty');

var isUnitlessNumber = CSSProperty.isUnitlessNumber;

/**
* Convert a value into the proper css writable value. The `styleName` name
* name should be logical (no hyphens), as specified
* Convert a value into the proper css writable value. The style name `name`
* should be logical (no hyphens), as specified
* in `CSSProperty.isUnitlessNumber`.
*
* @param {string} styleName CSS property name such as `topMargin`.
* @param {string} name CSS property name such as `topMargin`.
* @param {*} value CSS property value such as `10px`.
* @return {string} Normalized style value with dimensions applied.
*/
function dangerousStyleValue(styleName, value) {
function dangerousStyleValue(name, value) {
// Note that we've removed escapeTextForBrowser() calls here since the
// whole string will be escaped when the attribute is injected into
// the markup. If you provide unsafe user data here they can inject
Expand All @@ -47,7 +49,8 @@ function dangerousStyleValue(styleName, value) {
}

var isNonNumeric = isNaN(value);
if (isNonNumeric || value === 0 || CSSProperty.isUnitlessNumber[styleName]) {
if (isNonNumeric || value === 0 ||
isUnitlessNumber.hasOwnProperty(name) && isUnitlessNumber[name]) {
return '' + value; // cast to string
}

Expand Down
7 changes: 5 additions & 2 deletions src/core/ReactCompositeComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,9 @@ function validateTypeDef(Constructor, typeDef, location) {
}

function validateMethodOverride(proto, name) {
var specPolicy = ReactCompositeComponentInterface[name];
var specPolicy = ReactCompositeComponentInterface.hasOwnProperty(name) ?
ReactCompositeComponentInterface[name] :
null;

// Disallow overriding of base class methods unless explicitly allowed.
if (ReactCompositeComponentMixin.hasOwnProperty(name)) {
Expand Down Expand Up @@ -1011,7 +1013,8 @@ var ReactCompositeComponentMixin = {
var props = merge(newProps);
var defaultProps = this._defaultProps;
for (var propName in defaultProps) {
if (typeof props[propName] === 'undefined') {
if (!props.hasOwnProperty(propName) ||
typeof props[propName] === 'undefined') {
props[propName] = defaultProps[propName];
}
}
Expand Down
5 changes: 3 additions & 2 deletions src/event/EventPluginRegistry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ function recomputePluginOrdering() {
*/
function publishEventForPlugin(dispatchConfig, PluginModule, eventName) {
invariant(
!EventPluginRegistry.eventNameDispatchConfigs[eventName],
!EventPluginRegistry.eventNameDispatchConfigs.hasOwnProperty(eventName),
'EventPluginHub: More than one plugin attempted to publish the same ' +
'event name, `%s`.',
eventName
Expand Down Expand Up @@ -200,7 +200,8 @@ var EventPluginRegistry = {
continue;
}
var PluginModule = injectedNamesToPlugins[pluginName];
if (namesToPlugins[pluginName] !== PluginModule) {
if (!namesToPlugins.hasOwnProperty(pluginName) ||
namesToPlugins[pluginName] !== PluginModule) {
invariant(
!namesToPlugins[pluginName],
'EventPluginRegistry: Cannot inject two different event plugins ' +
Expand Down