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.

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

#722@minor: Add timeStamp property to Event. #723

Conversation

aripalo
Copy link
Contributor

@aripalo aripalo commented Feb 7, 2023

Fix #722 by implementing timeStamp property on Event.

According to specification (see MDN and Spec) the timeStamp property is described as follows:

This value is the number of milliseconds elapsed from the beginning of the time origin until the event was created. If the global object is Window, the time origin is the moment the user clicked on the link, or the script that initiated the loading of the document. In a worker, the time origin is the moment of creation of the worker.

My proposed solution is to use performance.timeOrigin as the time origin to resolve the relative Event timeStamp value.

Copy link
Owner

@capricorn86 capricorn86 left a comment

Choose a reason for hiding this comment

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

Nice contribution ⭐ Found some minor things.

packages/happy-dom/src/event/Event.ts Outdated Show resolved Hide resolved
packages/happy-dom/test/event/Event.test.ts Outdated Show resolved Hide resolved
@aripalo
Copy link
Contributor Author

aripalo commented Feb 9, 2023

I had to do a little bit of "massaging" for the timeStamp test to work. Used this solution from StackOverflow.

There's still some other tests failing in my fork after merging the upstream changes (bringing in Turborepo etc), but I'll try to figure them out.

@aripalo
Copy link
Contributor Author

aripalo commented Feb 9, 2023

I'm a bit stuck as now there's a circular dependency apparently due to use of performance 🤔

 FAIL  test/nodes/html-element/HTMLElement.test.ts
  ● Test suite failed to run

    TypeError: Converting circular structure to JSON
        --> starting at object with constructor 'HTMLDocument'
        |     property 'childNodes' -> object with constructor 'Array'
        |     index 0 -> object with constructor 'DocumentType'
        --- property 'ownerDocument' closes the circle
        at stringify (<anonymous>)

      at messageParent (../../node_modules/jest-worker/build/workers/messageParent.js:29:19)

I'll need to investigate this more a bit later.

@capricorn86
Copy link
Owner

I'm a bit stuck as now there's a circular dependency apparently due to use of performance thinking

 FAIL  test/nodes/html-element/HTMLElement.test.ts
  ● Test suite failed to run

    TypeError: Converting circular structure to JSON
        --> starting at object with constructor 'HTMLDocument'
        |     property 'childNodes' -> object with constructor 'Array'
        |     index 0 -> object with constructor 'DocumentType'
        --- property 'ownerDocument' closes the circle
        at stringify (<anonymous>)

      at messageParent (../../node_modules/jest-worker/build/workers/messageParent.js:29:19)

I'll need to investigate this more a bit later.

@aripalo usually this happens when toEqual() is used on a DOM element when it doesn't match. Then Jest is trying to convert the DOM element to JSON, but as DOM elements have circular references it fails.

I have tried to solve this in multiple places by changing from toEqual() to:
expect(element === otherElement).toBe(true)

I will try to make a better solution in the future. Maybe use Vitest instead and perhaps create a custom toEqual().

@capricorn86
Copy link
Owner

@aripalo I managed to find the problem with the failing unit test now. I have fixed it and pushed in two commits. I accidentally used the wrong user. It became a generated user, but I guess it is fine 😅

@capricorn86 capricorn86 self-requested a review February 13, 2023 12:18
@capricorn86 capricorn86 merged commit 5f304c7 into capricorn86:master Feb 13, 2023
@aripalo
Copy link
Contributor Author

aripalo commented Feb 14, 2023

@capricorn86 sweet! I was a bit stuck with this as I'm not really familiar with internals of Happy DOM and haven't had the time to dive into it lately.

Thanks for merging! 🙌 (and thanks for this library, really great stuff!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

timeStamp property missing from Event & CustomEvent
3 participants