-
Notifications
You must be signed in to change notification settings - Fork 51k
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.
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
Changes from 26 commits
a2f57e0
92f1e1c
52166d9
a5bb048
660e770
a84a2e6
db7e13d
74a7d9d
7bb6fa4
55a1e3c
23d406b
8a2651b
ae33345
9bec8b1
af292bc
1a093e5
9d6d1dd
dc1e6c2
6fa57fb
333d3d7
632c96c
b26e31f
ed4f899
4da5c57
91acb79
3cf8e44
1fe88e2
7e6dc19
7f67c45
97ea2b4
5d641c2
fead37f
77afc53
c198d82
3b0d45b
7509c6d
a59042e
39b142e
1c86699
b043bfb
37ccabe
8fcf649
4bd3b44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,8 +19,10 @@ import sanitizeURL from '../shared/sanitizeURL'; | |
| import { | ||
| disableJavaScriptURLs, | ||
| enableTrustedTypesIntegration, | ||
| enableCustomElementPropertySupport, | ||
| } from 'shared/ReactFeatureFlags'; | ||
| import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion'; | ||
| import {getCustomElementEventHandlersFromNode} from './ReactDOMComponentTree'; | ||
|
|
||
| import type {PropertyInfo} from '../shared/DOMProperty'; | ||
|
|
||
|
|
@@ -149,9 +151,52 @@ export function setValueForProperty( | |
| if (shouldIgnoreAttribute(name, propertyInfo, isCustomComponentTag)) { | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| enableCustomElementPropertySupport && | ||
| isCustomComponentTag && | ||
|
josepharhar marked this conversation as resolved.
|
||
| name[0] === 'o' && | ||
| name[1] === 'n' | ||
| ) { | ||
| let eventName = name.replace(/Capture$/, ''); | ||
| const useCapture = name !== eventName; | ||
|
josepharhar marked this conversation as resolved.
|
||
| const nameLower = eventName.toLowerCase(); | ||
| if (nameLower in node) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| eventName = nameLower; | ||
| } | ||
| eventName = eventName.slice(2); | ||
|
|
||
| const listenersObjName = eventName + (useCapture ? 'true' : 'false'); | ||
| const listeners = getCustomElementEventHandlersFromNode(node); | ||
| const alreadyHadListener = listeners[listenersObjName]; | ||
|
|
||
| if (typeof value === 'function' || alreadyHadListener) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Thoughts?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'));
This doesn't seem like consistent behavior. Some possible options:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I know that Sebastian seemed objected to reserving everything that starts with 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| listeners[listenersObjName] = value; | ||
| const proxy = useCapture ? fireEventProxyCapture : fireEventProxy; | ||
| 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 && | ||
|
josepharhar marked this conversation as resolved.
|
||
| name in (node: any) | ||
| ) { | ||
| (node: any)[name] = value; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should have test coverage verifying that this does not get set in cases where we don't expect. E.g. if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing this out! |
||
| return; | ||
| } | ||
|
|
||
| // If the prop isn't in the special list, treat it as a simple attribute. | ||
| if (isCustomComponentTag || propertyInfo === null) { | ||
| if (isAttributeNameSafe(name)) { | ||
|
|
@@ -216,3 +261,11 @@ export function setValueForProperty( | |
| } | ||
| } | ||
| } | ||
|
|
||
| function fireEventProxy(e: Event) { | ||
|
josepharhar marked this conversation as resolved.
Outdated
|
||
| getCustomElementEventHandlersFromNode(this)[e.type + 'false'](e); | ||
| } | ||
|
|
||
| function fireEventProxyCapture(e: Event) { | ||
| getCustomElementEventHandlersFromNode(this)[e.type + 'true'](e); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.