-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix: avoid registering non-function event-handlers on custom-elements #4121
Conversation
📊 Tachometer Benchmark ResultsSummaryA summary of the benchmark results will show here once they finish. ResultsThe full results of your benchmarks will show here once they finish. |
Size Change: +46 B (0%) Total Size: 57 kB
ℹ️ View Unchanged
|
The failure here is interesting, basically we have a fallthrough case now that we shield for the value being a function in either the old or new case. When we are hydrating and have the test where we perform |
I worry that this will prevent us from supporting EventListener Objects in the future, in addition to the issue you noted with set/unset. An idea I had while looking at this: what if we hoisted the if (name === 'style') {
// set/diff style
return;
}
if (
!isSvg &&
name !== 'width' &&
name !== 'height' &&
name !== 'href' &&
name !== 'list' &&
name !== 'form' &&
name !== 'tabIndex' &&
name !== 'download' &&
name !== 'rowSpan' &&
name !== 'colSpan' &&
name in dom
) {
try {
dom[name] = value == null ? '' : value;
return;
} catch (e) {
// property assignment failed, fall back to attribute
}
}
if (name[0] === 'o' && name[1] === 'n' && name.indexOf('-') === -1) {
// if we got here, we know `onxyz` is not a property defined on `dom` and there was no dash
// set/diff event listener
return;
}
if (typeof value === 'function') return;
// fall through to setting as attribute
if (value != null && (value !== false || name[4] === '-')) {
dom.setAttribute(name, value);
} else {
dom.removeAttribute(name);
} |
b22171f
to
f52e224
Compare
@developit the issue with that approach is that you'll fall into the Not too sure how we can support all three here
Where the first has to fall in our event-handler bucket and the latter two in our |
Fixes #4085
Prevent us from registering functions on custom-elements when the property starts with
on