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

[benbender]

event.currentTarget & event.target are read-only per spec. Setting either via property assignment may result in an error. This pr uses defineProperty to correctly set them.

Reference: https://dom.spec.whatwg.org/#interface-event

Closes #544 & #343

[capricorn86]

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?

[benbender]

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?

[capricorn86]

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;

		....
	}
        ...
}

[benbender]

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...

[capricorn86]

@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? 😅

[benbender]

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

[benbender]

Related: vitest-dev/vitest#1494

[capricorn86]

Hi @benbender! 🙂

Sorry for not looking in to this PR again earlier. It's been a lot going on in my private and work life.

I will do some tests and look into fixing this as soon as possible.

[benbender]

Hey,

thanks for getting back to this! No harm done - so take your time!

[benbender]

Hey @capricorn86, any reason that you assigned this one to me?

[capricorn86]

@benbender no reason except that I have assigned everybody that has created a PR to the PR to make it easier to look through the list of PRs

[benbender]

ah, sure. I was confused because you said you wanted to have look and I'm used to assign pr's to the person who's expected to do smth…

[capricorn86]

@benbender I have created another PR with a fix that hopefully solves the problem. This branch contains commits that does not follow the Happy DOM commit format, so I could not re-use it.

You will probably run into more problems if you let Vitest transpile external libraries. You need to change the transform config to only transform your app source code to avoid this problem.

    transformMode: {
      web: [/src\/\.[tj]sx?$/]
    }

Pull request:
#611