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.

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

Differentiate null and undefined in Custom Elements - removing sets to undefined #28716

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Apr 2, 2024

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. 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:

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

Deleting a property now means setting it to undefined.
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 2, 2024
@react-sizebot
Copy link

Comparing: 4eb241a...ba0c040

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 177.10 kB 177.12 kB +0.03% 55.05 kB 55.07 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 172.94 kB 172.96 kB +0.03% 53.91 kB 53.93 kB
facebook-www/ReactDOM-prod.classic.js = 592.68 kB 592.69 kB +0.02% 103.96 kB 103.97 kB
facebook-www/ReactDOM-prod.modern.js = 574.37 kB 574.38 kB +0.02% 100.98 kB 101.00 kB
test_utils/ReactAllWarnings.js Deleted 64.93 kB 0.00 kB Deleted 16.24 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 64.93 kB 0.00 kB Deleted 16.24 kB 0.00 kB

Generated by 🚫 dangerJS against ba0c040

Copy link
Member

@kassens kassens left a comment

Choose a reason for hiding this comment

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

I think this is a good compromise and lets custom components decide between explicit nulls vs. defaults.

@sebmarkbage sebmarkbage merged commit 48ec17b into facebook:main Apr 2, 2024
38 checks passed
github-actions bot pushed a commit that referenced this pull request Apr 2, 2024
…o 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

DiffTrain build for [48ec17b](48ec17b)
@josepharhar
Copy link
Contributor

So it sounds like the tl;dr is that custom elements should have setters for properties they care about and that undefined is considered the default? Sounds good! I feel like it could be good to write up some kind of authoring guidelines for writing custom elements that are compatible with React, but it's been so long since I've worked on this that I'm not sure what I would write.

EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…o undefined (facebook#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 facebook#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team React 19
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Removal of custom element property sets it to null rather than undefined
5 participants