| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
We had a similar issue when introducing the WHATWG URL parser which is why we have so many test-whatwg-url-* tests and don't rely so much on the WPT tests for those. I suspect we'll have to do the same.
Yeah, I suspect we should actually file this as an issue with the EventTarget standard itself. The following test case actually crashes Chrome for me and Firefox throws with an error about too much recursion: const et = new EventTarget();
et.addEventListener('foo', () => et.dispatchEvent(new Event('foo')));
et.dispatchEvent(new Event('foo'));It's an obvious footgun that really shouldn't be allowed. Note that we currently have the same issue with EventEmitter in that we allow ee.on('foo', () => ee.emit('foo')) which throws a max call stack exceeded error. It's a deeper problem with EventTarget because the error is just pushed to uncaught error and does not necessarily stop the progression of the code in browsers. The ERR_EVENT_RECURSION may not be fully spec compliant here but it prevents these issues and should be kept. |
Sorry, something went wrong.
Roger, I'll start porting the whatwg tests that route. I'm also checking with the #whatwg people on IRC to see if they're interesting in upstreaming those tests (for example: tests directly testing EventTarget rather than document) I think I fixed the root cause for the recursion issue in #33624 - it's fine to dispatch events from events with the same type as long as they're not the same isntance. If you'd like we can add a "max call depth" or similar check though I would rather not and have the user just reach "Maximum call stack exceeded" like any other non-event-related recusion and like browsers do. I'll add a "WIP" to the title in the meantime (at least until #33624 and some other fixes land). |
Sorry, something went wrong.
|
@jasnell any feelings regarding just porting all the WPTs in one PR (with a pass/fail status, a bunch failing) rather than make a lot of smaller changes? |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Commit Queue failed- Loading data for nodejs/node/pull/33621 ✔ Done loading data for nodejs/node/pull/33621 ----------------------------------- PR info ------------------------------------ Title events: Start porting WPT tests (#33621) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch benjamingr:event-target-wpt-start -> nodejs:master Labels events, eventtarget, test Commits 1 - events: port some wpt tests Committers 1 - Benjamin Gruenbaum PR-URL: https://github.com/nodejs/node/pull/33621 Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/33621 Reviewed-By: James M Snell -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - events: port some wpt tests ✖ Last GitHub CI failed ℹ Last Full PR CI on 2020-11-05T18:04:15Z: https://ci.nodejs.org/job/node-test-pull-request/34100/ - Querying data for job/node-test-pull-request/34100/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Thu, 28 May 2020 17:18:14 GMT ✔ Approvals: 1 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/33621#pullrequestreview-421144116 -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu Commit Queue action: https://github.com/nodejs/node/actions/runs/348132668 |
Sorry, something went wrong.
|
I'll land it manually |
Sorry, something went wrong.
PR-URL: #33621 Reviewed-By: James M Snell <jasnell@gmail.com>
|
this creates conflicts with tests added in #34169 |
Sorry, something went wrong.
PR-URL: #33621 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #33621 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #33621 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #33621 Reviewed-By: James M Snell <jasnell@gmail.com>
| Back | FazBrowse Home | New Git URL |
I tried porting the WPT tests through git node wpt - it worked pretty well initially but there were several big issues:
I started down the path of implementing a document, window, appendChild and then decided it would probably be easier to just manually port these tests after changing them to terms Node understands.
Ideally, we could contribute these back to WPT as less-tried-to-document tests for EventTarget.
For example where the test does document.addEventListener I do let document = new EventEmitter() first.
When I tried porting https://github.com/web-platform-tests/wpt/blob/master/dom/events/AddEventListenerOptions-once.html#L36-L57 it threw ERR_EVENT_RECURSION indicating that that behavior is probably not spec compliant. @jasnell any objections to changing it to align with the spec and allow dispatching an event while in an event handler for the same handler?
Checklist