Skip to content

Commit 48ec17b

Browse files
authored
Differentiate null and undefined in Custom Elements - removing sets to undefined (#28716)
In React DOM, in general, we don't differentiate between `null` and `undefined` because we expect to target DOM APIs. When we're setting a property on a Custom Element, in the new heuristic, the goal is to allow passing whatever data type instead of normalizing it. Switching between `undefined` and `null` as an explicit value should therefore be respected. However, in this mode if `undefined` is used for the initial value, we don't actually set the property at all. If passing `null` we will now initialize it to the value `null`. Meaning `undefined` kind of represents the default. ### Removing Properties There is a pretty complex edge case which is what should happen when a prop used to exist but was removed from the props object. This doesn't have any kind of defined semantics. It really should mean - return to "default". Because in the declarative world it means the same as if it was just created - i.e. we can't just leave it as it was. The closest might be `delete object.property` but that's not really the intended way that properties on custom elements / classes are supposed to operate. Additionally, for a property to even hit our heuristic it must pass the `in` test and must exist to being with so the default must have a value. Since the point of these properties is to contain any kind of type, there isn't really a conceptual default value. E.g. a numeric default value might be zero `0` while a default string might be empty `""` and default object might `null`. Additionally, the conceptual default can really be initialized to anything. There's also varied precedence in the ecosystem here and really no consensus. Anything we pick would be kind of wrong, so we used to just pick `null`. _The safest way to consume a Custom Element is to always pass the same set of props._ JS does have a concept of a "default value" though and that is described as the value `undefined`. That's why default argument / object property initializers are initialized if the value is `undefined`. The problem with using `undefined` as value is that [you shouldn't actually ever set the value of a class property to `undefined`](https://twitter.com/sebmarkbage/status/1774082540296388752). A property should always be initialized to some value. It can't be left missing and shouldn't be initialized to the value `undefined` for hidden class optimizations. If we just mutate it to be `undefined` it would be potentially bad for perf and shouldn't really be the value after removing property - it should be returned to default. Every property should really have a setter to be useful since it is what is used to trigger reactivity when it changes. Sometimes you can just use the properties passively when something else happens but most of the time it should be a setter but to reach parity with DOM it should really be always so that the active value can be normalized. Those setters can have default argument initializers to represent what the default value should be. Therefore Custom Element properties should be used like this: ```js class CustomElement extends HTMLElement { _textLabel = ''; _price = 0; _items = null; constructor() { super(); } set textLabel(value = '') { this._textLabel = value; } get textLabel() { return this._textLabel; } set price(value = 0) { this._price = value; } get price() { return this._price; } set items(value = null) { this._items = value; } get items() { return this._items; } } ``` The default initializer can be used to initialize a value back to its original default when `undefined` is passed to it. Therefore, we pass `undefined`, not because we expect that to be the value of a property but because that's the value that represents "return to default". This fixes #28203 but not really for the reason specified in the issue. We don't expect you to actually store the `undefined` value but to use a setter to set the property to something else that represents the default. When we initialize the element the first time, we won't set anything if it's the value `undefined` so we assume that the property initializers running in the constructor is going to set the same default value as if we set the property to `undefined`. cc @josepharhar
1 parent 4ecea96 commit 48ec17b

File tree

2 files changed

+76
-7
lines changed

2 files changed

+76
-7
lines changed

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1301,7 +1301,7 @@ export function setInitialProperties(
13011301
continue;
13021302
}
13031303
const propValue = props[propKey];
1304-
if (propValue == null) {
1304+
if (propValue === undefined) {
13051305
continue;
13061306
}
13071307
setPropOnCustomElement(
@@ -1310,7 +1310,7 @@ export function setInitialProperties(
13101310
propKey,
13111311
propValue,
13121312
props,
1313-
null,
1313+
undefined,
13141314
);
13151315
}
13161316
return;
@@ -1741,14 +1741,14 @@ export function updateProperties(
17411741
const lastProp = lastProps[propKey];
17421742
if (
17431743
lastProps.hasOwnProperty(propKey) &&
1744-
lastProp != null &&
1744+
lastProp !== undefined &&
17451745
!nextProps.hasOwnProperty(propKey)
17461746
) {
17471747
setPropOnCustomElement(
17481748
domElement,
17491749
tag,
17501750
propKey,
1751-
null,
1751+
undefined,
17521752
nextProps,
17531753
lastProp,
17541754
);
@@ -1760,7 +1760,7 @@ export function updateProperties(
17601760
if (
17611761
nextProps.hasOwnProperty(propKey) &&
17621762
nextProp !== lastProp &&
1763-
(nextProp != null || lastProp != null)
1763+
(nextProp !== undefined || lastProp !== undefined)
17641764
) {
17651765
setPropOnCustomElement(
17661766
domElement,

packages/react-dom/src/__tests__/DOMPropertyOperations-test.js

Lines changed: 71 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1230,7 +1230,7 @@ describe('DOMPropertyOperations', () => {
12301230
});
12311231
customelement.dispatchEvent(new Event('customevent'));
12321232
expect(oncustomevent).toHaveBeenCalledTimes(2);
1233-
expect(customelement.oncustomevent).toBe(null);
1233+
expect(customelement.oncustomevent).toBe(undefined);
12341234
expect(customelement.getAttribute('oncustomevent')).toBe(null);
12351235
});
12361236

@@ -1290,12 +1290,33 @@ describe('DOMPropertyOperations', () => {
12901290
await act(() => {
12911291
root.render(<my-custom-element />);
12921292
});
1293-
expect(customElement.foo).toBe(null);
1293+
expect(customElement.foo).toBe(undefined);
12941294
await act(() => {
12951295
root.render(<my-custom-element foo={myFunction} />);
12961296
});
12971297
expect(customElement.foo).toBe(myFunction);
12981298
});
1299+
1300+
it('switching between null and undefined should update a property', async () => {
1301+
const container = document.createElement('div');
1302+
document.body.appendChild(container);
1303+
const root = ReactDOMClient.createRoot(container);
1304+
await act(() => {
1305+
root.render(<my-custom-element foo={undefined} />);
1306+
});
1307+
const customElement = container.querySelector('my-custom-element');
1308+
customElement.foo = undefined;
1309+
1310+
await act(() => {
1311+
root.render(<my-custom-element foo={null} />);
1312+
});
1313+
expect(customElement.foo).toBe(null);
1314+
1315+
await act(() => {
1316+
root.render(<my-custom-element foo={undefined} />);
1317+
});
1318+
expect(customElement.foo).toBe(undefined);
1319+
});
12991320
});
13001321

13011322
describe('deleteValueForProperty', () => {
@@ -1349,5 +1370,53 @@ describe('DOMPropertyOperations', () => {
13491370
});
13501371
expect(container.firstChild.getAttribute('size')).toBe('5px');
13511372
});
1373+
1374+
it('custom elements should remove by setting undefined to restore defaults', async () => {
1375+
const container = document.createElement('div');
1376+
document.body.appendChild(container);
1377+
const root = ReactDOMClient.createRoot(container);
1378+
await act(() => {
1379+
root.render(<my-custom-element />);
1380+
});
1381+
const customElement = container.querySelector('my-custom-element');
1382+
1383+
// Non-setter but existing property to active the `in` heuristic
1384+
customElement.raw = 1;
1385+
1386+
// Install a setter to activate the `in` heuristic
1387+
Object.defineProperty(customElement, 'object', {
1388+
set: function (value = null) {
1389+
this._object = value;
1390+
},
1391+
get: function () {
1392+
return this._object;
1393+
},
1394+
});
1395+
1396+
Object.defineProperty(customElement, 'string', {
1397+
set: function (value = '') {
1398+
this._string = value;
1399+
},
1400+
get: function () {
1401+
return this._string;
1402+
},
1403+
});
1404+
1405+
const obj = {};
1406+
await act(() => {
1407+
root.render(<my-custom-element raw={2} object={obj} string="hi" />);
1408+
});
1409+
expect(customElement.raw).toBe(2);
1410+
expect(customElement.object).toBe(obj);
1411+
expect(customElement.string).toBe('hi');
1412+
1413+
// Removing the properties should reset to defaults by passing undefined
1414+
await act(() => {
1415+
root.render(<my-custom-element />);
1416+
});
1417+
expect(customElement.raw).toBe(undefined);
1418+
expect(customElement.object).toBe(null);
1419+
expect(customElement.string).toBe('');
1420+
});
13521421
});
13531422
});

0 commit comments

Comments
 (0)