Skip to content

Begin replacing propertyInfo.* access with functions #11733

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

Closed
wants to merge 16 commits into from
Closed

Begin replacing propertyInfo.* access with functions #11733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
e8d8042
Replace .attributeNamespace with a method
gaearon Nov 30, 2017
dcef72c
Remove .propertyName
gaearon Nov 30, 2017
8366004
Replace .attributeName with getAttributeName()
gaearon Nov 30, 2017
c9ca797
Replace MUST_USE_PROPERTY with shouldUseProperty()
gaearon Dec 1, 2017
0c067a0
Stop exporting `properties` from DOMProperty
gaearon Dec 1, 2017
bf74f67
Replace .has*Value flags with functions
gaearon Dec 1, 2017
2423bd6
Replace getPropertyInfo() with isWhitelisted()
gaearon Dec 1, 2017
1336ef3
Move shouldIgnoreValue() into DOMProperty
gaearon Dec 1, 2017
240d071
Make shouldIgnoreValue() more efficient again
gaearon Dec 1, 2017
650d731
Remove old comment
gaearon Dec 1, 2017
e5f2e0a
Remove masking completely
gaearon Dec 1, 2017
e0a6d44
Flowify DOMProperty
gaearon Dec 1, 2017
63703c2
Inline property configs in propertyTypes map
Dec 1, 2017
d4495a0
Merge branch 'master' into dom-inline-more
gaearon Dec 1, 2017
82a0d09
Merge branch 'master' into dom-inline-more
gaearon Dec 1, 2017
c236b23
Minor tweaks
gaearon Dec 1, 2017
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: 37 additions & 47 deletions packages/react-dom/src/client/DOMPropertyOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,14 @@ import {
ATTRIBUTE_NAME_START_CHAR,
ID_ATTRIBUTE_NAME,
ROOT_ATTRIBUTE_NAME,
getPropertyInfo,
getAttributeName,
getAttributeNamespace,
isWhitelisted,
hasBooleanValue,
hasOverloadedBooleanValue,
shouldIgnoreValue,
shouldSetAttribute,
shouldUseProperty,
} from '../shared/DOMProperty';
import warning from 'fbjs/lib/warning';

Expand All @@ -20,38 +26,26 @@ import warning from 'fbjs/lib/warning';
var VALID_ATTRIBUTE_NAME_REGEX = new RegExp(
'^[' + ATTRIBUTE_NAME_START_CHAR + '][' + ATTRIBUTE_NAME_CHAR + ']*$',
);
var illegalAttributeNameCache = {};
var validatedAttributeNameCache = {};
var illegalAttributeNameCache = new Set();
var validatedAttributeNameCache = new Set();
function isAttributeNameSafe(attributeName) {
if (validatedAttributeNameCache.hasOwnProperty(attributeName)) {
if (validatedAttributeNameCache.has(attributeName)) {
return true;
}
if (illegalAttributeNameCache.hasOwnProperty(attributeName)) {
if (illegalAttributeNameCache.has(attributeName)) {
return false;
}
if (VALID_ATTRIBUTE_NAME_REGEX.test(attributeName)) {
validatedAttributeNameCache[attributeName] = true;
validatedAttributeNameCache.add(attributeName);
return true;
}
illegalAttributeNameCache[attributeName] = true;
illegalAttributeNameCache.add(attributeName);
if (__DEV__) {
warning(false, 'Invalid attribute name: `%s`', attributeName);
}
return false;
}

// shouldIgnoreValue() is currently duplicated in DOMMarkupOperations.
// TODO: Find a better place for this.
function shouldIgnoreValue(propertyInfo, value) {
return (
value == null ||
(propertyInfo.hasBooleanValue && !value) ||
(propertyInfo.hasNumericValue && isNaN(value)) ||
(propertyInfo.hasPositiveNumericValue && value < 1) ||
(propertyInfo.hasOverloadedBooleanValue && value === false)
);
}

/**
* Operations for dealing with DOM properties.
*/
Expand All @@ -71,22 +65,21 @@ export function setAttributeForRoot(node) {
*/
export function getValueForProperty(node, name, expected) {
if (__DEV__) {
var propertyInfo = getPropertyInfo(name);
if (propertyInfo) {
if (propertyInfo.mustUseProperty) {
return node[propertyInfo.propertyName];
if (isWhitelisted(name)) {
if (shouldUseProperty(name)) {
return node[name];
} else {
var attributeName = propertyInfo.attributeName;
var attributeName = getAttributeName(name);

var stringValue = null;

if (propertyInfo.hasOverloadedBooleanValue) {
if (hasOverloadedBooleanValue(name)) {
if (node.hasAttribute(attributeName)) {
var value = node.getAttribute(attributeName);
if (value === '') {
return true;
}
if (shouldIgnoreValue(propertyInfo, expected)) {
if (shouldIgnoreValue(name, expected)) {
return value;
}
if (value === '' + expected) {
Expand All @@ -95,12 +88,12 @@ export function getValueForProperty(node, name, expected) {
return value;
}
} else if (node.hasAttribute(attributeName)) {
if (shouldIgnoreValue(propertyInfo, expected)) {
if (shouldIgnoreValue(name, expected)) {
// We had an attribute but shouldn't have had one, so read it
// for the error message.
return node.getAttribute(attributeName);
}
if (propertyInfo.hasBooleanValue) {
if (hasBooleanValue(name)) {
// If this was a boolean, it doesn't matter what the value is
// the fact that we have it is the same as the expected.
return expected;
Expand All @@ -112,7 +105,7 @@ export function getValueForProperty(node, name, expected) {
stringValue = node.getAttribute(attributeName);
}

if (shouldIgnoreValue(propertyInfo, expected)) {
if (shouldIgnoreValue(name, expected)) {
return stringValue === null ? expected : stringValue;
} else if (stringValue === '' + expected) {
return expected;
Expand Down Expand Up @@ -153,26 +146,24 @@ export function getValueForAttribute(node, name, expected) {
* @param {*} value
*/
export function setValueForProperty(node, name, value) {
var propertyInfo = getPropertyInfo(name);

if (propertyInfo && shouldSetAttribute(name, value)) {
if (shouldIgnoreValue(propertyInfo, value)) {
if (isWhitelisted(name) && shouldSetAttribute(name, value)) {
if (shouldIgnoreValue(name, value)) {
deleteValueForProperty(node, name);
return;
} else if (propertyInfo.mustUseProperty) {
} else if (shouldUseProperty(name)) {
// Contrary to `setAttribute`, object properties are properly
// `toString`ed by IE8/9.
node[propertyInfo.propertyName] = value;
node[name] = value;
} else {
var attributeName = propertyInfo.attributeName;
var namespace = propertyInfo.attributeNamespace;
var attributeName = getAttributeName(name);
var namespace = getAttributeNamespace(name);
// `setAttribute` with objects becomes only `[object]` in IE8/9,
// ('' + value) makes it output the correct toString()-value.
Copy link
Contributor

@nhunzaker nhunzaker Dec 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is this true in IE9?

screen shot 2017-11-30 at 8 02 51 pm

Copy link
Contributor

Choose a reason for hiding this comment

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

Also valueOf:

screen shot 2017-11-30 at 8 03 38 pm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe only IE8?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Regardless, don't want to change the logic here, only the structure :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally. It's just nice to look at this code again with a fresh set of eyes. I'll file an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

#11735

if (namespace) {
node.setAttributeNS(namespace, attributeName, '' + value);
} else if (
propertyInfo.hasBooleanValue ||
(propertyInfo.hasOverloadedBooleanValue && value === true)
hasBooleanValue(name) ||
(hasOverloadedBooleanValue(name) && value === true)
) {
node.setAttribute(attributeName, '');
} else {
Expand Down Expand Up @@ -217,17 +208,16 @@ export function deleteValueForAttribute(node, name) {
* @param {string} name
*/
export function deleteValueForProperty(node, name) {
var propertyInfo = getPropertyInfo(name);
if (propertyInfo) {
if (propertyInfo.mustUseProperty) {
var propName = propertyInfo.propertyName;
if (propertyInfo.hasBooleanValue) {
node[propName] = false;
if (isWhitelisted(name)) {
if (shouldUseProperty(name)) {
if (hasBooleanValue(name)) {
node[name] = false;
} else {
node[propName] = '';
node[name] = '';
}
} else {
node.removeAttribute(propertyInfo.attributeName);
const attributeName = getAttributeName(name);
node.removeAttribute(attributeName);
}
} else {
node.removeAttribute(name);
Expand Down
12 changes: 8 additions & 4 deletions packages/react-dom/src/client/ReactDOMFiberComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ import setTextContent from './setTextContent';
import {listenTo, trapBubbledEvent} from '../events/ReactBrowserEventEmitter';
import * as CSSPropertyOperations from '../shared/CSSPropertyOperations';
import {Namespaces, getIntrinsicNamespace} from '../shared/DOMNamespaces';
import {getPropertyInfo, shouldSetAttribute} from '../shared/DOMProperty';
import {
getAttributeName,
isWhitelisted,
shouldSetAttribute,
} from '../shared/DOMProperty';
import assertValidProps from '../shared/assertValidProps';
import {DOCUMENT_NODE, DOCUMENT_FRAGMENT_NODE} from '../shared/HTMLNodeType';
import isCustomComponent from '../shared/isCustomComponent';
Expand Down Expand Up @@ -952,7 +956,6 @@ export function diffHydratedProperties(
} else if (__DEV__) {
// Validate that the properties correspond to their expected values.
var serverValue;
var propertyInfo;
if (suppressHydrationWarning) {
// Don't bother comparing. We're ignoring all these warnings.
} else if (
Expand Down Expand Up @@ -995,9 +998,10 @@ export function diffHydratedProperties(
warnForPropDifference(propKey, serverValue, nextProp);
}
} else if (shouldSetAttribute(propKey, nextProp)) {
if ((propertyInfo = getPropertyInfo(propKey))) {
if (isWhitelisted(propKey)) {
const attributeName = getAttributeName(propKey);
// $FlowFixMe - Should be inferred as not undefined.
extraAttributeNames.delete(propertyInfo.attributeName);
extraAttributeNames.delete(attributeName);
serverValue = DOMPropertyOperations.getValueForProperty(
domElement,
propKey,
Expand Down
41 changes: 16 additions & 25 deletions packages/react-dom/src/server/DOMMarkupOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,12 @@ import {
ATTRIBUTE_NAME_START_CHAR,
ID_ATTRIBUTE_NAME,
ROOT_ATTRIBUTE_NAME,
getPropertyInfo,
getAttributeName,
isWhitelisted,
hasBooleanValue,
hasOverloadedBooleanValue,
shouldAttributeAcceptBooleanValue,
shouldIgnoreValue,
shouldSetAttribute,
} from '../shared/DOMProperty';
import quoteAttributeValueForBrowser from './quoteAttributeValueForBrowser';
Expand All @@ -22,38 +26,26 @@ import warning from 'fbjs/lib/warning';
var VALID_ATTRIBUTE_NAME_REGEX = new RegExp(
'^[' + ATTRIBUTE_NAME_START_CHAR + '][' + ATTRIBUTE_NAME_CHAR + ']*$',
);
var illegalAttributeNameCache = {};
var validatedAttributeNameCache = {};
var illegalAttributeNameCache = new Set();
var validatedAttributeNameCache = new Set();
function isAttributeNameSafe(attributeName) {
if (validatedAttributeNameCache.hasOwnProperty(attributeName)) {
if (validatedAttributeNameCache.has(attributeName)) {
return true;
}
if (illegalAttributeNameCache.hasOwnProperty(attributeName)) {
if (illegalAttributeNameCache.has(attributeName)) {
return false;
}
if (VALID_ATTRIBUTE_NAME_REGEX.test(attributeName)) {
validatedAttributeNameCache[attributeName] = true;
validatedAttributeNameCache.add(attributeName);
return true;
}
illegalAttributeNameCache[attributeName] = true;
illegalAttributeNameCache.add(attributeName);
if (__DEV__) {
warning(false, 'Invalid attribute name: `%s`', attributeName);
}
return false;
}

// shouldIgnoreValue() is currently duplicated in DOMPropertyOperations.
// TODO: Find a better place for this.
function shouldIgnoreValue(propertyInfo, value) {
return (
value == null ||
(propertyInfo.hasBooleanValue && !value) ||
(propertyInfo.hasNumericValue && isNaN(value)) ||
(propertyInfo.hasPositiveNumericValue && value < 1) ||
(propertyInfo.hasOverloadedBooleanValue && value === false)
);
}

/**
* Operations for dealing with DOM properties.
*/
Expand All @@ -80,15 +72,14 @@ export function createMarkupForRoot() {
* @return {?string} Markup string, or null if the property was invalid.
*/
export function createMarkupForProperty(name, value) {
var propertyInfo = getPropertyInfo(name);
if (propertyInfo) {
if (shouldIgnoreValue(propertyInfo, value)) {
if (isWhitelisted(name)) {
if (shouldIgnoreValue(name, value)) {
return '';
}
var attributeName = propertyInfo.attributeName;
var attributeName = getAttributeName(name);
if (
propertyInfo.hasBooleanValue ||
(propertyInfo.hasOverloadedBooleanValue && value === true)
hasBooleanValue(name) ||
(hasOverloadedBooleanValue(name) && value === true)
) {
return attributeName;
} else if (
Expand Down
Loading