Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add custom element property support behind a flag #22184
Add custom element property support behind a flag #22184
Changes from 25 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
There are no files selected for viewing
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 namedclick
instead ofClick
, but only if the element has anonclick
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.There was a problem hiding this comment.
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}
andvalue={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?
There was a problem hiding this comment.
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:
but this throws:
This doesn't seem like consistent behavior.
Some possible options:
console.error
. We still shouldn't have crashes like the above though.There was a problem hiding this comment.
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 doone={'foo'}
for example). I believe that this behavior of looking forfunction
s 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 thefunction
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
oncustomevent
is not supposed to set theoncustomevent
property even when it exists, we should have a test verifying this. That test should specify what happens when the value type changes midway (e.g. function -> string and back).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out!
I added some tests with a variety of switching between nothing, string, and function.