lib: add brand checks to AbortController and AbortSignal - #37720
lib: add brand checks to AbortController and AbortSignal#37720MattiasBuelens wants to merge 6 commits into
Conversation
Sorry, something went wrong.
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
This LGTM!
Sorry, something went wrong.
|
Can you add tests? |
Sorry, something went wrong.
|
@aduh95 Ah, I see, there's a test/parallel/test-abortcontroller.js file. Sure, I'll add some tests. They'll kind of duplicate the WPT IDL tests, but as long as we can't run those against Node yet, I suppose duplicating them makes sense. Actually, my initial assumption was wrong: the IDL tests do not test against "bad" this contexts in method calls. So we definitely need our own tests. 🙂 |
Sorry, something went wrong.
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>
aduh95
left a comment
There was a problem hiding this comment.
LGTM
IDL tests do not test against "bad" this contexts in method calls
It may be worth opening a PR to add those tests upstream to make sure the behaviour stays consistent.
Sorry, something went wrong.
|
Okay, so my correction on my initial assumption was wrong. 😅 As Domenic pointed out, the IDL harness does try to call getters and methods with a "bad" receiver to check if they throw a TypeError. So we don't need to upstream any tests, this is already covered by the WPT IDL tests. My apologies for the confusion. Once Node can run the IDL tests, we can remove these newly added tests. But for now, they are necessary, so I'll leave them in. |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: #37720 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #37720 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#37720 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Web IDL specifies that both attributes and operations of IDL interfaces should throw a TypeError if this does not implement the interface. These brand checks were missing for AbortController and AbortSignal (as noticed here), so I added them.
Unfortunately, this is not straightforward to test. WPT has an IDL test for this, but it's bunched up together with other IDL tests for EventTarget, Event and CustomEvent. Node supports the first two but not the last one, so the test would still fail. 😕 Suggestions are welcome!