Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: update event.target & event.currentTarget via defineProperty. #545

Closed
wants to merge 5 commits into from
Closed

fix: update event.target & event.currentTarget via defineProperty. #545

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b4e5e23
fix: update currenTarget via defineProperty. Fixes #544 & #343.
benbender Jul 18, 2022
daf779e
fix: set event via defineProperties
benbender Jul 18, 2022
98fcc43
Merge branch 'master' into fix/set-event-target
benbender Jul 25, 2022
23a212b
Merge branch 'master' into fix/set-event-target
capricorn86 Oct 6, 2022
a4f5a94
Merge branch 'master' into fix/set-event-target
benbender Oct 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions packages/happy-dom/src/event/EventTarget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,16 @@ export default abstract class EventTarget implements IEventTarget {
*/
public dispatchEvent(event: Event): boolean {
if (!event.target) {
event.target = this;
Object.defineProperty(event, "target", {
value: this,
writable: false
});
}

event.currentTarget = this;
Object.defineProperty(event, "currentTarget", {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @benbender! 🙂

Thank you for contributing!

I am a bit worried about the performance impact of calling defineProperty() on each event and element where it bubbles. Using defineProperty() may change the shape of the object, which could take away optimizations in the V8 engine.

Maybe we can do a solution where we use getters in the Event class, which returns _currentTarget and _target that can be modified from here?

We can also do some performance testing and see how it affects it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey 👋 ,

I gave your idea about the getters a quick shot and it did not work. The problem is that jest-testing-library uses lib.dom.d.ts where target/currentTarget are readonly. So as soon as we are writing directly to them and types are checked at runtime (vitest), this will fail :/

Tbh, I didn't even thought about performance in this context. I would be happy to do some microbenchmarks if there is some infrastructure I could use. Do you have anything at hand which could be repurposed?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is weird that jest-testing-library runs the compiler on the happy-dom internal code 🤔

Maybe it is possible to do something like this?

Event.ts

export default class Event {
        ...
	public _currentTarget: IEventTarget = null;
	public _target: IEventTarget = null;

	/**
	 * Returns target.
	 *
	 * @returns Target.
	 */
	public get target(): string {
	      return this._target;
	}

	/**
	 * Returns current target.
	 *
	 * @returns Current target.
	 */
	public get currentTarget(): string {
	      return this._currentTarget;
	}
	
	...
}

EventTarget.ts

export default class EventTarget {
        ...
	public dispatchEvent(event: Event): boolean {
		if (!event._target) {
			event._target = this;
		}

		event._currentTarget = this;

		....
	}
        ...
}

Copy link
Author

@benbender benbender Jul 23, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly this won't help as its a typing issue. So whenever you try to write to .target/.currentTarget, it will fail. This problem is independent of the implementation in happy-dom. The problem originates from the (correct) types in lib.dom.d.ts, as they mark those props as readonly. The only fix (at least as far as I can see) is to not write directly to those readonly-props in the interfaces. That's also why the defineProperty()-approach works...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benbender I am not sure if you missed the "_" (underline) before the properties. My idea was that we perhaps can write to other properties instead and then return them from getters that are never changed? Then we will not risk any performance penalty of using Object.defineProperty().

I would also like to understand why vitest or jest-testing-library are checking the types at runtime in the internal code of happy-dom. Do you have some example code I can run? 😅

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Repro: https://github.com/benbender/happy-dom-repro-544

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: vitest-dev/vitest#1494

value: this,
writable: false
});

const onEventName = 'on' + event.type.toLowerCase();

Expand Down