Skip to content

Commit eeabb73

Browse files
authored
Refactor DOM Bindings Completely Off of DOMProperty Meta Programming (#26546)
There are four places we have special cases based off the DOMProperty config: 1) DEV-only: ReactDOMUnknownPropertyHook warns for passing booleans to non-boolean attributes. We just need a simple list of all properties that are affected by that. We could probably move this in under setProp instead and have it covered by that list. 2) DEV-only: Hydration. This just needs to read the value from an attribute and compare it to what we'd expect to see if it was rendered on the client. This could use some simplification/unification of the code but I decided to just keep it simple and duplicated since code size isn't an issue. 3) DOMServerFormatConfig pushAttribute: This just maps the special case to how to emit it as a HTML attribute. 4) ReactDOMComponent setProp: This just maps the special case to how to emit it as setAttribute or removeAttribute. Basically we just have to remember to keep pushAttribute and setProp aligned. There's only one long switch in prod per environment. This just turns it all to a giant simple switch statement with string cases. This is in theory the most optimizable since syntactically all the information for a hash table is there. However, unfortunately we know that most VMs don't optimize this very well and instead just turn them into a bunch of ifs. JSC is best. We can minimize the cost by just moving common attribute to the beginning of the list. If we shipped this, maybe VMs will get it together to start optimizing this case but there's a chicken and egg problem here and the game theory reality is that we probably don't want to regress. Therefore, I intend to do a follow up after landing this which reintroduces an object indirection for simple property aliases. That should be enough to make the remaining cases palatable. I'll also extract the most common attributes to the beginning or separate ifs. Ran attribute-behavior fixture and the table is the same.
1 parent 0ba4d7b commit eeabb73

File tree

7 files changed

+2696
-1099
lines changed

7 files changed

+2696
-1099
lines changed

packages/react-dom-bindings/src/client/CSSPropertyOperations.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,21 @@ export function createDangerousStringForStyles(styles) {
7272
* @param {object} styles
7373
*/
7474
export function setValueForStyles(node, styles) {
75+
if (styles != null && typeof styles !== 'object') {
76+
throw new Error(
77+
'The `style` prop expects a mapping from style properties to values, ' +
78+
"not a string. For example, style={{marginRight: spacing + 'em'}} when " +
79+
'using JSX.',
80+
);
81+
}
82+
if (__DEV__) {
83+
if (styles) {
84+
// Freeze the next style object so that we can assume it won't be
85+
// mutated. We have already warned for this in the past.
86+
Object.freeze(styles);
87+
}
88+
}
89+
7590
const style = node.style;
7691
for (const styleName in styles) {
7792
if (!styles.hasOwnProperty(styleName)) {

packages/react-dom-bindings/src/client/DOMPropertyOperations.js

Lines changed: 29 additions & 299 deletions
Original file line numberDiff line numberDiff line change
@@ -7,181 +7,14 @@
77
* @flow
88
*/
99

10-
import {
11-
BOOLEAN,
12-
OVERLOADED_BOOLEAN,
13-
NUMERIC,
14-
POSITIVE_NUMERIC,
15-
} from '../shared/DOMProperty';
16-
1710
import isAttributeNameSafe from '../shared/isAttributeNameSafe';
18-
import sanitizeURL from '../shared/sanitizeURL';
1911
import {
2012
enableTrustedTypesIntegration,
2113
enableCustomElementPropertySupport,
22-
enableFilterEmptyStringAttributesDOM,
2314
} from 'shared/ReactFeatureFlags';
2415
import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion';
2516
import {getFiberCurrentPropsFromNode} from './ReactDOMComponentTree';
2617

27-
import type {PropertyInfo} from '../shared/DOMProperty';
28-
29-
/**
30-
* Get the value for a property on a node. Only used in DEV for SSR validation.
31-
* The "expected" argument is used as a hint of what the expected value is.
32-
* Some properties have multiple equivalent values.
33-
*/
34-
export function getValueForProperty(
35-
node: Element,
36-
name: string,
37-
expected: mixed,
38-
propertyInfo: PropertyInfo,
39-
): mixed {
40-
if (__DEV__) {
41-
const attributeName = propertyInfo.attributeName;
42-
43-
if (!node.hasAttribute(attributeName)) {
44-
// shouldRemoveAttribute
45-
switch (typeof expected) {
46-
case 'function':
47-
case 'symbol': // eslint-disable-line
48-
return expected;
49-
case 'boolean': {
50-
if (!propertyInfo.acceptsBooleans) {
51-
return expected;
52-
}
53-
}
54-
}
55-
switch (propertyInfo.type) {
56-
case BOOLEAN: {
57-
if (!expected) {
58-
return expected;
59-
}
60-
break;
61-
}
62-
case OVERLOADED_BOOLEAN: {
63-
if (expected === false) {
64-
return expected;
65-
}
66-
break;
67-
}
68-
case NUMERIC: {
69-
if (isNaN(expected)) {
70-
return expected;
71-
}
72-
break;
73-
}
74-
case POSITIVE_NUMERIC: {
75-
if (isNaN(expected) || (expected: any) < 1) {
76-
return expected;
77-
}
78-
break;
79-
}
80-
}
81-
if (enableFilterEmptyStringAttributesDOM) {
82-
if (propertyInfo.removeEmptyString && expected === '') {
83-
if (__DEV__) {
84-
if (name === 'src') {
85-
console.error(
86-
'An empty string ("") was passed to the %s attribute. ' +
87-
'This may cause the browser to download the whole page again over the network. ' +
88-
'To fix this, either do not render the element at all ' +
89-
'or pass null to %s instead of an empty string.',
90-
name,
91-
name,
92-
);
93-
} else {
94-
console.error(
95-
'An empty string ("") was passed to the %s attribute. ' +
96-
'To fix this, either do not render the element at all ' +
97-
'or pass null to %s instead of an empty string.',
98-
name,
99-
name,
100-
);
101-
}
102-
}
103-
return expected;
104-
}
105-
}
106-
return expected === undefined ? undefined : null;
107-
}
108-
109-
// Even if this property uses a namespace we use getAttribute
110-
// because we assume its namespaced name is the same as our config.
111-
// To use getAttributeNS we need the local name which we don't have
112-
// in our config atm.
113-
const value = node.getAttribute(attributeName);
114-
115-
if (expected == null) {
116-
// We had an attribute but shouldn't have had one, so read it
117-
// for the error message.
118-
return value;
119-
}
120-
121-
// shouldRemoveAttribute
122-
switch (typeof expected) {
123-
case 'function':
124-
case 'symbol': // eslint-disable-line
125-
return value;
126-
}
127-
switch (propertyInfo.type) {
128-
case BOOLEAN: {
129-
if (expected) {
130-
// If this was a boolean, it doesn't matter what the value is
131-
// the fact that we have it is the same as the expected.
132-
// As long as it's positive.
133-
return expected;
134-
}
135-
return value;
136-
}
137-
case OVERLOADED_BOOLEAN: {
138-
if (value === '') {
139-
return true;
140-
}
141-
if (expected === false) {
142-
// We had an attribute but shouldn't have had one, so read it
143-
// for the error message.
144-
return value;
145-
}
146-
break;
147-
}
148-
case NUMERIC: {
149-
if (isNaN(expected)) {
150-
// We had an attribute but shouldn't have had one, so read it
151-
// for the error message.
152-
return value;
153-
}
154-
break;
155-
}
156-
case POSITIVE_NUMERIC: {
157-
if (isNaN(expected) || (expected: any) < 1) {
158-
// We had an attribute but shouldn't have had one, so read it
159-
// for the error message.
160-
return value;
161-
}
162-
break;
163-
}
164-
}
165-
if (__DEV__) {
166-
checkAttributeStringCoercion(expected, name);
167-
}
168-
if (propertyInfo.sanitizeURL) {
169-
// We have already verified this above.
170-
// eslint-disable-next-line react-internal/safe-string-coercion
171-
if (value === '' + (sanitizeURL(expected): any)) {
172-
return expected;
173-
}
174-
return value;
175-
}
176-
// We have already verified this above.
177-
// eslint-disable-next-line react-internal/safe-string-coercion
178-
if (value === '' + (expected: any)) {
179-
return expected;
180-
}
181-
return value;
182-
}
183-
}
184-
18518
/**
18619
* Get the value for a attribute on a node. Only used in DEV for SSR validation.
18720
* The third argument is used as a hint of what the expected value is. Some
@@ -271,138 +104,6 @@ export function getValueForAttributeOnCustomComponent(
271104
}
272105
}
273106

274-
/**
275-
* Sets the value for a property on a node.
276-
*
277-
* @param {DOMElement} node
278-
* @param {string} name
279-
* @param {*} value
280-
*/
281-
export function setValueForProperty(
282-
node: Element,
283-
propertyInfo: PropertyInfo,
284-
value: mixed,
285-
) {
286-
const attributeName = propertyInfo.attributeName;
287-
288-
if (value === null) {
289-
node.removeAttribute(attributeName);
290-
return;
291-
}
292-
293-
// shouldRemoveAttribute
294-
switch (typeof value) {
295-
case 'undefined':
296-
case 'function':
297-
case 'symbol': // eslint-disable-line
298-
node.removeAttribute(attributeName);
299-
return;
300-
case 'boolean': {
301-
if (!propertyInfo.acceptsBooleans) {
302-
node.removeAttribute(attributeName);
303-
return;
304-
}
305-
}
306-
}
307-
if (enableFilterEmptyStringAttributesDOM) {
308-
if (propertyInfo.removeEmptyString && value === '') {
309-
if (__DEV__) {
310-
if (attributeName === 'src') {
311-
console.error(
312-
'An empty string ("") was passed to the %s attribute. ' +
313-
'This may cause the browser to download the whole page again over the network. ' +
314-
'To fix this, either do not render the element at all ' +
315-
'or pass null to %s instead of an empty string.',
316-
attributeName,
317-
attributeName,
318-
);
319-
} else {
320-
console.error(
321-
'An empty string ("") was passed to the %s attribute. ' +
322-
'To fix this, either do not render the element at all ' +
323-
'or pass null to %s instead of an empty string.',
324-
attributeName,
325-
attributeName,
326-
);
327-
}
328-
}
329-
node.removeAttribute(attributeName);
330-
return;
331-
}
332-
}
333-
334-
switch (propertyInfo.type) {
335-
case BOOLEAN:
336-
if (value) {
337-
node.setAttribute(attributeName, '');
338-
} else {
339-
node.removeAttribute(attributeName);
340-
return;
341-
}
342-
break;
343-
case OVERLOADED_BOOLEAN:
344-
if (value === true) {
345-
node.setAttribute(attributeName, '');
346-
} else if (value === false) {
347-
node.removeAttribute(attributeName);
348-
} else {
349-
if (__DEV__) {
350-
checkAttributeStringCoercion(value, attributeName);
351-
}
352-
node.setAttribute(attributeName, (value: any));
353-
}
354-
return;
355-
case NUMERIC:
356-
if (!isNaN(value)) {
357-
if (__DEV__) {
358-
checkAttributeStringCoercion(value, attributeName);
359-
}
360-
node.setAttribute(attributeName, (value: any));
361-
} else {
362-
node.removeAttribute(attributeName);
363-
}
364-
break;
365-
case POSITIVE_NUMERIC:
366-
if (!isNaN(value) && (value: any) >= 1) {
367-
if (__DEV__) {
368-
checkAttributeStringCoercion(value, attributeName);
369-
}
370-
node.setAttribute(attributeName, (value: any));
371-
} else {
372-
node.removeAttribute(attributeName);
373-
}
374-
break;
375-
default: {
376-
if (__DEV__) {
377-
checkAttributeStringCoercion(value, attributeName);
378-
}
379-
let attributeValue;
380-
// `setAttribute` with objects becomes only `[object]` in IE8/9,
381-
// ('' + value) makes it output the correct toString()-value.
382-
if (enableTrustedTypesIntegration) {
383-
if (propertyInfo.sanitizeURL) {
384-
attributeValue = (sanitizeURL(value): any);
385-
} else {
386-
attributeValue = (value: any);
387-
}
388-
} else {
389-
// We have already verified this above.
390-
// eslint-disable-next-line react-internal/safe-string-coercion
391-
attributeValue = '' + (value: any);
392-
if (propertyInfo.sanitizeURL) {
393-
attributeValue = sanitizeURL(attributeValue);
394-
}
395-
}
396-
const attributeNamespace = propertyInfo.attributeNamespace;
397-
if (attributeNamespace) {
398-
node.setAttributeNS(attributeNamespace, attributeName, attributeValue);
399-
} else {
400-
node.setAttribute(attributeName, attributeValue);
401-
}
402-
}
403-
}
404-
}
405-
406107
export function setValueForAttribute(
407108
node: Element,
408109
name: string,
@@ -439,6 +140,35 @@ export function setValueForAttribute(
439140
}
440141
}
441142

143+
export function setValueForNamespacedAttribute(
144+
node: Element,
145+
namespace: string,
146+
name: string,
147+
value: mixed,
148+
) {
149+
if (value === null) {
150+
node.removeAttribute(name);
151+
return;
152+
}
153+
switch (typeof value) {
154+
case 'undefined':
155+
case 'function':
156+
case 'symbol':
157+
case 'boolean': {
158+
node.removeAttribute(name);
159+
return;
160+
}
161+
}
162+
if (__DEV__) {
163+
checkAttributeStringCoercion(value, name);
164+
}
165+
node.setAttributeNS(
166+
namespace,
167+
name,
168+
enableTrustedTypesIntegration ? (value: any) : '' + (value: any),
169+
);
170+
}
171+
442172
export function setValueForPropertyOnCustomComponent(
443173
node: Element,
444174
name: string,

0 commit comments

Comments
 (0)