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

Make EventTarget Spec Compliant #34122

Closed
3 tasks
Ethan-Arrowood opened this issue Jun 29, 2020 · 12 comments
Closed
3 tasks

Make EventTarget Spec Compliant #34122

Ethan-Arrowood opened this issue Jun 29, 2020 · 12 comments
Labels
eventtarget Issues and PRs related to the EventTarget implementation.

Comments

@Ethan-Arrowood
Copy link
Contributor

Ethan-Arrowood commented Jun 29, 2020

Based on #34074 this issue tracks improving the internal EventTarget implementation to be achieve spec compliance.

From a previous comment these are some initial action items:

  • Add WPT.
  • Add other tests.
  • Go over the spec and fix deltas (and add tests)

Unless there is a better spec document I plan on using this one as reference: https://dom.spec.whatwg.org/#interface-eventtarget

WPT: https://github.com/web-platform-tests/wpt/tree/master/dom/events
EventEmitter vs EventTarget: https://docs.google.com/document/d/1NFARs04-4U_2y6Ssw9Lqu1GMXBUM981-NO9PLJWifTI/edit#

@Ethan-Arrowood
Copy link
Contributor Author

WPT branch: https://github.com/Ethan-Arrowood/node/tree/add-event-target-wpt

@benjamingr how do I add Events and EventTarget to global object for the test-events.js file?

@benjamingr
Copy link
Member

@benjamingr how do I add Events and EventTarget to global object for the test-events.js file?

You can just put them on global at the start of the test - though I would probably manually port the tests since the WPTs rely heavily on other parts of the DOM (like Element) being present

@Ethan-Arrowood
Copy link
Contributor Author

I followed the WPT guide found at /node/test/wpt/README.md and did the whole git node wpt dom/events thing. Should I now go test by test and modify them to fit Node? Or when you say manual do you mean not doing this method at all and just rewriting these tests one by one?

@benjamingr
Copy link
Member

@Ethan-Arrowood I tried that route and it didn't work. I meant not use git node wpt and port the tests manually (to /tests/parallel) because the browser DOM tests use window

@Ethan-Arrowood
Copy link
Contributor Author

Ah I see, okay sounds good I'll scrap this stuff then and begin the conversion. Would you like me to put up a pr when I have some or all of them done?

@benjamingr
Copy link
Member

Sure, that would be helpful :]

@Ethan-Arrowood
Copy link
Contributor Author

First test added @benjamingr draft pr #34169 - more will come soon 😄

@addaleax addaleax added the eventtarget Issues and PRs related to the EventTarget implementation. label Oct 4, 2020
@benjamingr
Copy link
Member

Hey, any status/updates on this?

@Ethan-Arrowood
Copy link
Contributor Author

I'll pin the open PR and give it some attention soon

@benjamingr
Copy link
Member

OK, let me know if you need any help with specifics.

@Ethan-Arrowood
Copy link
Contributor Author

Okay i've updated that pr, hopefully it can be merged soon.

@benjamingr do you have any ideas how I can be more organized in writing more WPT tests for this? The last time I tried writing more of them I realized a lot didn't really apply to Node (since they were very much DOM based).

Together if we could compile a list of all the tests that would need to be transferred over id be happy to start making my way through them 😁

@benjamingr
Copy link
Member

Port things that are Node based to be EventEmitter based probably

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
eventtarget Issues and PRs related to the EventTarget implementation.
Projects
None yet
Development

No branches or pull requests

3 participants