Skip to content

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

Merged
merged 3 commits into from
Jul 11, 2025
Merged

EventTarget constructor #860

merged 3 commits into from
Jul 11, 2025

Conversation

sjorsdonkers
Copy link
Contributor

@sjorsdonkers sjorsdonkers commented Jul 8, 2025

Depends on: lightpanda-io/libdom#30
Support new EventTarget()

This is likely for custom elements.

@karlseguin
Copy link
Collaborator

I don't understand the toInterface issue?

@sjorsdonkers
Copy link
Contributor Author

@karlseguin

I don't understand the toInterface issue?

If toInterface is called with an event and EventTarget (created using the constructor), we are not able to detect that these are created as such.
Therefore the toInterface function is just going to cast the EventTarget to a Node which would be wrong.
Other eventtargets like xhr or abort_signal use the eventGetInternalType. This works because we can set this internal type ourselves for those events.

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 Event should set the internalType, or is that constructor also used for Nodes?
I'll check what that does.

@karlseguin
Copy link
Collaborator

Yes, it would crash, and yes, it could be called (e.g. if they called this.currentTarget within the callback).

In the above snippet, where / how is new EventTarget being called (presumably in dispatchEvent ?).

With AbortSignal and XMLHttpRequestEventTarget, I was able to avoid adding a type to libdom's event target, because they are always created together, so I could use the event's type. But, for this case, it does seem like the discriminator needs to be on the event_target itself.

@sjorsdonkers sjorsdonkers requested a review from karlseguin July 10, 2025 14:03
if (try parser.eventTargetInternalType(et) == .plain) {
return .{ .plain = et };
}

Copy link
Collaborator

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.

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 agree, Just wanted confirmation of the chosen solution first before doing the others.
Added the others and cleanup here: 14eeffd

@karlseguin
Copy link
Collaborator

LGTM

@sjorsdonkers sjorsdonkers merged commit 5123697 into main Jul 11, 2025
10 checks passed
@sjorsdonkers sjorsdonkers deleted the EventTarget-constructor branch July 11, 2025 07:55
@github-actions github-actions bot locked and limited conversation to collaborators Jul 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants