-
Notifications
You must be signed in to change notification settings - Fork 239
EventTarget constructor #860
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
Conversation
I don't understand the toInterface issue? |
If In this case however the user Js code is creating both the Event and the EventTarget directly, so the eventInternalType is not set. I don't know if toInterface is ever called with those user created events, but if it is it will likely crash. Perhaps the constructor of |
Yes, it would crash, and yes, it could be called (e.g. if they called In the above snippet, where / how is With AbortSignal and XMLHttpRequestEventTarget, I was able to avoid adding a |
src/browser/dom/event_target.zig
Outdated
if (try parser.eventTargetInternalType(et) == .plain) { | ||
return .{ .plain = et }; | ||
} | ||
|
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 understand keeping the PR focused is good. But, if the goal is to migrate AbortSignal and XMLHttpRequestEventTarget to this, then it might be worth including that change now. It's a good way to make sure the new field/behavior is capable of handling all know (and thus hopefully future) cases. Your call.
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 agree, Just wanted confirmation of the chosen solution first before doing the others.
Added the others and cleanup here: 14eeffd
LGTM |
Depends on: lightpanda-io/libdom#30
Support
new EventTarget()
This is likely for custom elements.