| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM + please add the reference - thanks for this!
Sorry, something went wrong.
|
This test is what is failing the CI: https://github.com/nodejs/node/pull/34169/checks?check_run_id=831452407#step:11:10071 Any ideas? |
Sorry, something went wrong.
|
New commit moves away from the count checking and reorganizes the tests using mustCall. This now deviates a bit from the WPT tests but I believe achieves the same thing. If this is not the intended change I can revert the last commit! One note; I tried implementing this: {
const document = new EventTarget();
const handler = common.mustCall(2)
// Both should only fire on first event
document.addEventListener('test', handler, { once: true });
document.addEventListener('test', handler, { once: true });
// Fire events
document.dispatchEvent(new Event('test'));
document.dispatchEvent(new Event('test'));
}But it fails - probably my own misunderstanding of common.mustCall. Maybe you two have some in sight here? |
Sorry, something went wrong.
|
It's failing because, unlike EventEmitter, a given function can only be added once. So the second document.addEventListener('test', handler, { once: true }); has no effect. Try changing it to document.addEventListener('test', handler.bind(), { once: true }); and it should hopefully work. |
Sorry, something went wrong.
|
Awesome worked like a charm! Thank you |
Sorry, something went wrong.
|
Hey! I have another test file ready to go. Should I push it to this PR/branch or include it in another branch? |
Sorry, something went wrong.
|
@Ethan-Arrowood ... just push it here. We can do another CI run on this and give it another day to land. |
Sorry, something went wrong.
|
@Ethan-Arrowood could you please squash/fixup the appropriate commits and remove the merge commit, our CI doesn't handle merge commits very well? This looks ready to land to me after a successful CI. |
Sorry, something went wrong.
|
Alright squashed and fixed up your comments. |
Sorry, something went wrong.
|
This pr #33621 introduces some conflicts with test-eventtarget-whatwg-once.js. I'm pushing another rebase that fixes some of that. I guess this PR needs to be reviewed again |
Sorry, something went wrong.
Sorry, something went wrong.
|
@aduh95 let me know if theres anything else I should do here |
Sorry, something went wrong.
|
@Ethan-Arrowood can you rebase to resolve the git conflict please? |
Sorry, something went wrong.
add reference comments for WPT tests convert to common mustCall Update test/parallel/test-eventtarget-whatwg-once.js Co-authored-by: James M Snell <jasnell@gmail.com> Update test/parallel/test-eventtarget-whatwg-passive.js Co-authored-by: James M Snell <jasnell@gmail.com> convert other tests to utilize common mustcall improve test with bind add customevent wpt add no-unused-vars comment reorder header utilize common.mustcall remove internal and use global EventTarget
Sorry, something went wrong.
Add WPT AddEventListenerOptions-once test. PR-URL: #34169 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Add WPT AddEventListenerOptions-once test. PR-URL: #34169 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
| Back | FazBrowse Home | New Git URL |
Checklist
First of many PRs adding WPT tests for EventTarget (and eventually AbortController)