Skip to content

Add custom element property support behind a flag #22184

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

Merged
merged 43 commits into from
Dec 8, 2021
Merged

Add custom element property support behind a flag #22184

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
a2f57e0
custom element props
josepharhar Jan 11, 2021
92f1e1c
custom element events
josepharhar Jan 8, 2021
52166d9
use function type for on*
josepharhar Aug 17, 2021
a5bb048
tests, htmlFor
josepharhar Aug 25, 2021
660e770
className
josepharhar Aug 25, 2021
a84a2e6
fix ReactDOMComponent-test
josepharhar Aug 26, 2021
db7e13d
started on adding feature flag
josepharhar Aug 26, 2021
74a7d9d
added feature flag to all feature flag files
josepharhar Aug 26, 2021
7bb6fa4
everything passes
josepharhar Aug 26, 2021
55a1e3c
tried to fix getPropertyInfo
josepharhar Aug 26, 2021
23d406b
used @gate and __experimental__
josepharhar Aug 27, 2021
8a2651b
remove flag gating for test which already passes
josepharhar Aug 27, 2021
ae33345
fix onClick test
josepharhar Aug 31, 2021
9bec8b1
add __EXPERIMENTAL__ to www flags, rename eventProxy
josepharhar Aug 31, 2021
af292bc
Add innerText and textContent to reservedProps
josepharhar Sep 14, 2021
1a093e5
Emit warning when assigning to read only properties in client
josepharhar Sep 28, 2021
9d6d1dd
Revert "Emit warning when assigning to read only properties in client"
josepharhar Sep 29, 2021
dc1e6c2
Emit warning when assigning to read only properties during hydration
josepharhar Sep 29, 2021
6fa57fb
yarn prettier-all
josepharhar Sep 29, 2021
333d3d7
Gate hydration warning test on flag
josepharhar Nov 4, 2021
632c96c
Merge with 2 months of upstream commits
josepharhar Nov 5, 2021
b26e31f
Fix gating in hydration warning test
josepharhar Nov 5, 2021
ed4f899
Fix assignment to boolean properties
josepharhar Nov 5, 2021
4da5c57
Replace _listeners with random suffix matching
josepharhar Nov 6, 2021
91acb79
Improve gating for hydration warning test
josepharhar Nov 6, 2021
3cf8e44
Add outerText and outerHTML to server warning properties
josepharhar Nov 18, 2021
1fe88e2
remove nameLower logic
josepharhar Nov 30, 2021
7e6dc19
fix capture event listener test
josepharhar Nov 30, 2021
7f67c45
Add coverage for changing custom event listeners
josepharhar Nov 30, 2021
97ea2b4
yarn prettier-all
josepharhar Nov 30, 2021
5d641c2
yarn lint --fix
josepharhar Nov 30, 2021
fead37f
replace getCustomElementEventHandlersFromNode with getFiberCurrentPro…
josepharhar Nov 30, 2021
77afc53
Remove previous value when adding event listener
josepharhar Dec 3, 2021
c198d82
flow, lint, prettier
josepharhar Dec 3, 2021
3b0d45b
Add dispatchEvent to make sure nothing crashes
josepharhar Dec 6, 2021
7509c6d
Add state change to reserved attribute tests
josepharhar Dec 6, 2021
a59042e
Add missing feature flag test gate
josepharhar Dec 6, 2021
39b142e
Reimplement SSR changes in ReactDOMServerFormatConfig
josepharhar Dec 7, 2021
1c86699
Test hydration for objects and functions
josepharhar Dec 7, 2021
b043bfb
add missing test gate
josepharhar Dec 7, 2021
37ccabe
remove extraneous comment
josepharhar Dec 7, 2021
8fcf649
Add attribute->property test
josepharhar Dec 7, 2021
4bd3b44
Merge with 4 weeks of upstream commits
josepharhar Dec 7, 2021
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
99 changes: 98 additions & 1 deletion packages/react-dom/src/__tests__/DOMPropertyOperations-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
'use strict';

// Set by `yarn test-fire`.
const {disableInputAttributeSyncing} = require('shared/ReactFeatureFlags');
const {disableInputAttributeSyncing, enableCustomElementPropertySupport} = require('shared/ReactFeatureFlags');

describe('DOMPropertyOperations', () => {
let React;
Expand Down Expand Up @@ -155,6 +155,103 @@ describe('DOMPropertyOperations', () => {
// Regression test for https://github.com/facebook/react/issues/6119
expect(container.firstChild.hasAttribute('value')).toBe(false);
});

if (enableCustomElementPropertySupport) {
it('custom element custom events lowercase', () => {
const oncustomevent = jest.fn();
function Test() {
return <my-custom-element oncustomevent={oncustomevent} />;
}
const container = document.createElement('div');
ReactDOM.render(<Test />, container);
container.querySelector('my-custom-element').dispatchEvent(new Event('customevent'));
expect(oncustomevent).toHaveBeenCalledTimes(1);
});

it('custom element custom events uppercase', () => {
const oncustomevent = jest.fn();
function Test() {
return <my-custom-element onCustomevent={oncustomevent} />;
}
const container = document.createElement('div');
ReactDOM.render(<Test />, container);
container.querySelector('my-custom-element').dispatchEvent(new Event('Customevent'));
expect(oncustomevent).toHaveBeenCalledTimes(1);
});

it('custom element custom event with dash in name', () => {
const oncustomevent = jest.fn();
function Test() {
return <my-custom-element oncustom-event={oncustomevent} />;
}
const container = document.createElement('div');
ReactDOM.render(<Test />, container);
container.querySelector('my-custom-element').dispatchEvent(new Event('custom-event'));
expect(oncustomevent).toHaveBeenCalledTimes(1);
});

it('custom element remove event handler', () => {
const oncustomevent = jest.fn();
function Test(props) {
return <my-custom-element oncustomevent={props.handler} />;
}

const container = document.createElement('div');
ReactDOM.render(<Test handler={oncustomevent} />, container);
const customElement = container.querySelector('my-custom-element');
customElement.dispatchEvent(new Event('customevent'));
expect(oncustomevent).toHaveBeenCalledTimes(1);

ReactDOM.render(<Test handler={false} />, container);
// Make sure that the second render didn't create a new element. We want
// to make sure removeEventListener actually gets called on the same element.
expect(customElement).toBe(customElement);
customElement.dispatchEvent(new Event('customevent'));
expect(oncustomevent).toHaveBeenCalledTimes(1);
});

it('custom elements shouldnt have non-functions for on* attributes treated as event listeners', () => {
const container = document.createElement('div');
ReactDOM.render(<my-custom-element
onstring={'hello'}
onobj={{hello: 'world'}}
onarray={['one', 'two']}
ontrue={true}
onfalse={false} />, container);
const customElement = container.querySelector('my-custom-element');
expect(customElement.getAttribute('onstring')).toBe('hello');
expect(customElement.getAttribute('onobj')).toBe('[object Object]');
expect(customElement.getAttribute('onarray')).toBe('one,two');
expect(customElement.getAttribute('ontrue')).toBe('true');
expect(customElement.getAttribute('onfalse')).toBe('false');
});

it('custom elements should still have onClick treated like regular elements', () => {
const eventhandler = jest.fn();
function Test() {
return <span onClick={eventhandler} />;
}
const container = document.createElement('div');
ReactDOM.render(<Test />, container);
container.querySelector('span').click();
// TODO why doesn't this test pass!? As far as I can tell from outside
// testing, this actually does work... maybe I'm missing some test setup
// included in DOMPluginEventSystem-test.internal.js?
expect(eventhandler).toHaveBeenCalledTimes(1);
});

it('custom elements should allow custom events with capture event listeners', () => {
const oncustomevent = jest.fn();
function Test() {
return <my-custom-element oncustomeventCapture={oncustomevent} />;
}
const container = document.createElement('div');
ReactDOM.render(<Test />, container);
container.querySelector('my-custom-element')
.dispatchEvent(new Event('customevent', {bubbles: false}));
expect(oncustomevent).toHaveBeenCalledTimes(1);
});
}
});

describe('deleteValueForProperty', () => {
Expand Down
15 changes: 13 additions & 2 deletions packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
'use strict';

const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils');
const ReactFeatureFlags = require('shared/ReactFeatureFlags');

let React;
let ReactDOM;
Expand All @@ -36,6 +37,7 @@ const {
resetModules,
itRenders,
clientCleanRender,
clientRenderOnServerString,
} = ReactDOMServerIntegrationUtils(initModules);

describe('ReactDOMServerIntegration', () => {
Expand Down Expand Up @@ -657,17 +659,26 @@ describe('ReactDOMServerIntegration', () => {
});

itRenders('className for custom elements', async render => {
const e = await render(<div is="custom-element" className="test" />, 0);
expect(e.getAttribute('className')).toBe('test');
if (ReactFeatureFlags.enableCustomElementPropertySupport) {
const e = await render(<div is="custom-element" className="test" />,
render === clientRenderOnServerString ? 1 : 0);
expect(e.getAttribute('className')).toBe(null);
expect(e.getAttribute('class')).toBe('test');
} else {
const e = await render(<div is="custom-element" className="test" />, 0);
expect(e.getAttribute('className')).toBe('test');
}
});

itRenders('htmlFor attribute on custom elements', async render => {
const e = await render(<div is="custom-element" htmlFor="test" />);
expect(e.getAttribute('htmlFor')).toBe('test');
expect(e.getAttribute('for')).toBe(null);
});

itRenders('for attribute on custom elements', async render => {
const e = await render(<div is="custom-element" for="test" />);
expect(e.getAttribute('htmlFor')).toBe(null);
expect(e.getAttribute('for')).toBe('test');
});

Expand Down
48 changes: 46 additions & 2 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 @@ -9,6 +9,7 @@

import {
getPropertyInfo,
getCustomElementPropertyInfo,
shouldIgnoreAttribute,
shouldRemoveAttribute,
isAttributeNameSafe,
Expand All @@ -19,6 +20,7 @@ import sanitizeURL from '../shared/sanitizeURL';
import {
disableJavaScriptURLs,
enableTrustedTypesIntegration,
enableCustomElementPropertySupport,
} from 'shared/ReactFeatureFlags';
import {isOpaqueHydratingObject} from './ReactDOMHostConfig';

Expand Down Expand Up @@ -139,15 +141,49 @@ export function setValueForProperty(
value: mixed,
isCustomComponentTag: boolean,
) {
const propertyInfo = getPropertyInfo(name);
let propertyInfo = getPropertyInfo(name);
if (shouldIgnoreAttribute(name, propertyInfo, isCustomComponentTag)) {
return;
}

if (enableCustomElementPropertySupport && isCustomComponentTag && name[0] === 'o' && name[1] === 'n') {
let eventName = name.replace(/Capture$/, '');
const useCapture = name !== eventName;
const nameLower = eventName.toLowerCase();
if (nameLower in node) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing this if block doesn't fail any tests. What is it for? Do we need a test for coverage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this over from these lines in preact: https://github.com/preactjs/preact/blob/dd1e281ddc6bf056aa6eaf5755b71112ef5011c5/src/diff/props.js#L88-L90

I suppose it's to make it so onClick listens for an event named click instead of Click, but only if the element has an onclick property. The comment suggests that this only matters for builtin elements, and since removing this line still makes the <my-custom-element onClick={...} /> test pass, it can be removed. I'll remove it.

eventName = nameLower;
}
eventName = eventName.slice(2);

const listenersObjName = eventName + (useCapture ? 'true' : 'false');
const alreadyHadListener = (node: any)._listeners && (node: any)._listeners[listenersObjName];

if (typeof value === 'function' || alreadyHadListener) {
Copy link
Collaborator

@gaearon gaearon Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm. So we do different things depending on the value type. This makes me a bit uneasy. I guess that's existing practice in Preact etc?

This makes me uneasy because props can always change midway. E.g. you get function on first render and something else on the second render. Do we have tests demonstrating what exactly happens when the type changes? The guarantee we try to preserve is that A -> B -> C -> D should have the same "end result" as just D, regardless of what A, B, C were. E.g. number -> string -> function -> number, or number -> function -> function -> string. If we can't guarantee that IMO we should at least warn. Or not support this.

(There might be some parallel in that we don't support "switching" between passing value={undefined} and value={str} to inputs. We warn in that case. At least we probably should make sure, whatever the behavior is, it probably shouldn't throw in just one of those cases.)

Thoughts?

Copy link
Collaborator

@gaearon gaearon Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Here's a concrete example.

This doesn't throw:

      const container = document.createElement('div');
      // ReactDOM.render(<Test handler={oncustomevent} />, container);
      ReactDOM.render(<Test handler="hello" />, container);
      const customElement = container.querySelector('my-custom-element');
      customElement.dispatchEvent(new Event('customevent'));

but this throws:

      const container = document.createElement('div');
      ReactDOM.render(<Test handler={oncustomevent} />, container);
      ReactDOM.render(<Test handler="hello" />, container);
      const customElement = container.querySelector('my-custom-element');
      customElement.dispatchEvent(new Event('customevent'));

TypeError: (0 , _ReactDOMComponentTree.getCustomElementEventHandlersFromNode)(...)[(e.type + "false")] is not a function

This doesn't seem like consistent behavior.

Some possible options:

  1. Fully consistent behavior, where the behavior matches the thing you set last. E.g. if you do event -> string, the behavior should be the same as if you set it to string right away. So no event attached, and nothing happens if it's dispatched. Essentially, treat passing non-function as a reason to remove the listener.
  2. Inconsistent behavior (e.g. whatever you pass first determines the behavior from that point on). But then passing something else should print a console.error. We still shouldn't have crashes like the above though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out this case! I like the fully consistent behavior option.

More on the function type behavior - Preact doesn't look at the type of the value passed in and unconditionally forwards it to addEventListener. Jason Miller endorsed this behavior by telling me that it supports the EventListener interface better because addEventListener can take objects in addition to just functions, and that nobody has ever complained about all properties with on being reserved.

I know that Sebastian seemed objected to reserving everything that starts with on (meaning you can't do one={'foo'} for example). I believe that this behavior of looking for functions strongly mitigates this issue, and I don't really see the EventListener interface being a big issue either. However, I don't exactly remember what Sebastian's last thoughts about this were, and I don't remember exactly when I decided to start looking for the function type. I did propose the behavior in this comment, and there wasn't really anyone disagreeing.

In the end, I think that we should go forward with this and do the Fully consistent behavior option. I'll plan on implementing it once I get through the rest of the comments in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some code here to implement the fully consistent option - when calling addEventListener, it will now try to figure out if we previously assigned the same prop as an attribute or property, in which case it will assign null into it.

if (!(node: any)._listeners) {
(node: any)._listeners = {};
}

(node: any)._listeners[listenersObjName] = value;
const proxy = useCapture ? eventProxyCapture : eventProxy;
if (value) {
if (!alreadyHadListener) {
node.addEventListener(eventName, proxy, useCapture);
}
} else {
node.removeEventListener(eventName, proxy, useCapture);
}
return;
}
}

if (shouldRemoveAttribute(name, value, propertyInfo, isCustomComponentTag)) {
value = null;
}
if (enableCustomElementPropertySupport && isCustomComponentTag) {
propertyInfo = getCustomElementPropertyInfo(name, node);
}
// If the prop isn't in the special list, treat it as a simple attribute.
if (isCustomComponentTag || propertyInfo === null) {
if (propertyInfo === null || (isCustomComponentTag && !enableCustomElementPropertySupport)) {
if (isAttributeNameSafe(name)) {
const attributeName = name;
if (value === null) {
Expand Down Expand Up @@ -204,3 +240,11 @@ export function setValueForProperty(
}
}
}

function eventProxy(e: Event) {
this._listeners[e.type + 'false'](e);
}

function eventProxyCapture(e: Event) {
this._listeners[e.type + 'true'](e);
}
9 changes: 6 additions & 3 deletions packages/react-dom/src/client/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import {
import {HTML_NAMESPACE, getIntrinsicNamespace} from '../shared/DOMNamespaces';
import {
getPropertyInfo,
getCustomElementPropertyInfo,
shouldIgnoreAttribute,
shouldRemoveAttribute,
} from '../shared/DOMProperty';
Expand All @@ -69,7 +70,7 @@ import {validateProperties as validateInputProperties} from '../shared/ReactDOMN
import {validateProperties as validateUnknownProperties} from '../shared/ReactDOMUnknownPropertyHook';
import {REACT_OPAQUE_ID_TYPE} from 'shared/ReactSymbols';

import {enableTrustedTypesIntegration} from 'shared/ReactFeatureFlags';
import {enableTrustedTypesIntegration, enableCustomElementPropertySupport} from 'shared/ReactFeatureFlags';
import {
mediaEventTypes,
listenToNonDelegatedEvent,
Expand Down Expand Up @@ -1004,7 +1005,9 @@ export function diffHydratedProperties(
) {
// Validate that the properties correspond to their expected values.
let serverValue;
const propertyInfo = getPropertyInfo(propKey);
const propertyInfo = isCustomComponentTag && enableCustomElementPropertySupport
? getCustomElementPropertyInfo(propKey, domElement)
: getPropertyInfo(propKey);
if (suppressHydrationWarning) {
// Don't bother comparing. We're ignoring all these warnings.
} else if (
Expand Down Expand Up @@ -1037,7 +1040,7 @@ export function diffHydratedProperties(
warnForPropDifference(propKey, serverValue, expectedStyle);
}
}
} else if (isCustomComponentTag) {
} else if (isCustomComponentTag && !enableCustomElementPropertySupport) {
// $FlowFixMe - Should be inferred as not undefined.
extraAttributeNames.delete(propKey.toLowerCase());
serverValue = getValueForAttribute(domElement, propKey, nextProp);
Expand Down
11 changes: 7 additions & 4 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 @@ -17,6 +17,7 @@ import {
} from '../shared/DOMProperty';
import sanitizeURL from '../shared/sanitizeURL';
import quoteAttributeValueForBrowser from './quoteAttributeValueForBrowser';
import {enableCustomElementPropertySupport} from 'shared/ReactFeatureFlags';

/**
* Operations for dealing with DOM properties.
Expand All @@ -29,12 +30,14 @@ import quoteAttributeValueForBrowser from './quoteAttributeValueForBrowser';
* @param {*} value
* @return {?string} Markup string, or null if the property was invalid.
*/
export function createMarkupForProperty(name: string, value: mixed): string {
const propertyInfo = getPropertyInfo(name);
if (name !== 'style' && shouldIgnoreAttribute(name, propertyInfo, false)) {
export function createMarkupForProperty(name: string, value: mixed, isCustomComponent: boolean): string {
const propertyInfo = enableCustomElementPropertySupport && isCustomComponent
? null
: getPropertyInfo(name);
if (name !== 'style' && shouldIgnoreAttribute(name, propertyInfo, isCustomComponent && enableCustomElementPropertySupport)) {
return '';
}
if (shouldRemoveAttribute(name, value, propertyInfo, false)) {
if (shouldRemoveAttribute(name, value, propertyInfo, isCustomComponent && enableCustomElementPropertySupport)) {
return '';
}
if (propertyInfo !== null) {
Expand Down
9 changes: 7 additions & 2 deletions packages/react-dom/src/server/ReactDOMServerFormatConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import type {ReactNodeList} from 'shared/ReactTypes';

import {Children} from 'react';

import {enableFilterEmptyStringAttributesDOM} from 'shared/ReactFeatureFlags';
import {enableFilterEmptyStringAttributesDOM, enableCustomElementPropertySupport} from 'shared/ReactFeatureFlags';

import type {
Destination,
Expand Down Expand Up @@ -1128,12 +1128,17 @@ function pushStartCustomElement(

let children = null;
let innerHTML = null;
for (const propKey in props) {
for (let propKey in props) {
if (hasOwnProperty.call(props, propKey)) {
const propValue = props[propKey];
if (propValue == null) {
continue;
}
if (enableCustomElementPropertySupport && propKey === 'className') {
// className gets rendered as class on the client, so it should be
// rendered as class on the server.
propKey = 'class';
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about htmlFor? Is that needed here?

Copy link
Contributor Author

@josepharhar josepharhar Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

className is special because all elements have a builtin className property which triggers the 'className' in node check when rendering on the client - meaning that rendering on the client will always use a property instead of an attribute for className. Without this, one of the automated tests was getting errors (I think during hydration?) due to this mismatch.
Alternatively we could make it so that when client rendering className is always set as an attribute instead of a property, but I think this makes more sense and it's what Preact is already doing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea this make sense because htmlFor doesn't have any semantics on custom element which are just Generic HTML elements.

There are a number of other ones that are case sensitive for similar reasons. contentEditable, tabIndex, etc. They probably work if used correctly, but it would be nice to add a warning if they use the wrong case.

There are some that just won't work like offsetHeight etc. Since those exist as properties but are read-only. Probably should ensure we have a nice warning about those and why they don't work. So that if they're rendered on the server, it doesn't look like they work.

Similarly there are some we have to be careful with like innerHTML and innerText. It shouldn't pass through as a property to custom elements - even in production so it can't be a DEV only error. Could be worth a test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised innerText and textContent aren't reserved properties in React. Doesn't seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a number of other ones that are case sensitive for similar reasons. contentEditable, tabIndex, etc. They probably work if used correctly, but it would be nice to add a warning if they use the wrong case.

contentEditable is interesting, it looks like the attribute can be either contentEditable or contenteditable, they both work, and the reflected object property is contentEditable.
So I guess if someone writes <my-custom-element contentEditable={true} /> or <my-custom-element contenteditable={true} /> they should both work fine, right...?

There are some that just won't work like offsetHeight etc. Since those exist as properties but are read-only. Probably should ensure we have a nice warning about those and why they don't work. So that if they're rendered on the server, it doesn't look like they work.

I mean, you can assign to offsetHeight from script and the browser doesn't complain about it... but yeah I guess it would be weird to have a state where you can assign to an attribute called offsetHeight when rendering on the server, but you can't on the client because it will always trigger the 'offsetHeight' in node check...

Should I consider every global attribute and every built in property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the read only properties like offsetHeight, I just pushed a commit which emits the warning: 1a093e5

However, the test fails because it throws an exception when assigning to the read only properties - but this exception doesn't occur in the browser or in JSDOM... what kind of browser/DOM setup is being used in these tests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

“use strict” throws, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow, today I learned...
What do you think we should do? Remove the test? Make it somehow get rid of the 'use strict'?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe if you render it on the server and hydrate it wouldn’t error.

The reason for the warning would be so that if you render it on the server, during development you should know not to.

So you can test hydrating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that makes sense!
I replaced the client rendering check with a hydration check and made a test for it.

}
switch (propKey) {
case 'children':
children = propValue;
Expand Down
15 changes: 10 additions & 5 deletions packages/react-dom/src/server/ReactPartialRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
disableModulePatternComponents,
enableSuspenseServerRenderer,
enableScopeAPI,
enableCustomElementPropertySupport,
} from 'shared/ReactFeatureFlags';

import {
Expand Down Expand Up @@ -376,12 +377,16 @@ function createOpenTagMarkup(
propValue = createMarkupForStyles(propValue);
}
let markup = null;
if (isCustomComponent) {
if (!RESERVED_PROPS.hasOwnProperty(propKey)) {
markup = createMarkupForCustomAttribute(propKey, propValue);
}
if (enableCustomElementPropertySupport) {
markup = createMarkupForProperty(propKey, propValue, isCustomComponent);
} else {
markup = createMarkupForProperty(propKey, propValue);
if (isCustomComponent) {
if (!RESERVED_PROPS.hasOwnProperty(propKey)) {
markup = createMarkupForCustomAttribute(propKey, propValue);
}
} else {
markup = createMarkupForProperty(propKey, propValue, false);
}
}
if (markup) {
ret += ' ' + markup;
Expand Down
19 changes: 19 additions & 0 deletions packages/react-dom/src/shared/DOMProperty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,25 @@ export function getPropertyInfo(name: string): PropertyInfo | null {
return properties.hasOwnProperty(name) ? properties[name] : null;
}

export function getCustomElementPropertyInfo(
name: string,
node: Element) {
if (name in (node: any)) {
const acceptsBooleans = (typeof (node: any)[name]) === 'boolean';
return {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems a bit slow to do for every property. Likely will need some refactoring. However, is this check also equivalent on the built-ins?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to name in (node: any) or (typeof (node: any)[name]) === 'boolean'?

acceptsBooleans,
type: acceptsBooleans ? BOOLEAN : STRING,
mustUseProperty: true,
propertyName: name,
attributeName: name,
attributeNamespace: null,
sanitizeURL: false,
removeEmptyString: false,
};
}
return null;
}

function PropertyInfoRecord(
name: string,
type: PropertyType,
Expand Down
6 changes: 6 additions & 0 deletions packages/shared/ReactFeatureFlags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,12 @@ export const deletedTreeCleanUpLevel = 3;
// Note that this should be an uncommon use case and can be avoided by using the transition API.
export const enableSuspenseLayoutEffectSemantics = true;

// Changes the behavior for rendering custom elements in both server rendering
// and client rendering, mostly to allow JSX attributes to apply to the custom
// element's object properties instead of only HTML attributes.
// https://github.com/facebook/react/issues/11347
export const enableCustomElementPropertySupport = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's ok to set this to __EXPERIMENTAL__ and it'll be on in the experimental builds but the the "next" stable release channel which 18 is on.

That way it'll have some test coverage in CI on and you get a build to experiment with.

Copy link
Contributor Author

@josepharhar josepharhar Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Awesome!!
Should I set it to __EXPERIMENTAL__ in all of the separate feature flag files I had to add this flag to?


// --------------------------
// Future APIs to be deprecated
// --------------------------
Expand Down
Loading