-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
lib: implement interface converter and use it for AbortSignal validation #54965
base: main
Are you sure you want to change the base?
Conversation
dd6e62f
to
8f1cafc
Compare
createSequenceConverter, | ||
} = require('internal/webidl'); | ||
|
||
const { | ||
validateAbortSignal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we update validateAbortSignal
– or remove it if we no longer need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried updating it, but it breaks some tests, as it is used variously in other modules. Some modules treat EventTarget
with { aborted: boolean; }
as a valid AbortSignal
, and its test is written in that way.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54965 +/- ##
==========================================
- Coverage 88.07% 88.03% -0.05%
==========================================
Files 652 652
Lines 183542 183778 +236
Branches 35862 35855 -7
==========================================
+ Hits 161653 161783 +130
- Misses 15144 15249 +105
- Partials 6745 6746 +1
|
@@ -275,20 +277,34 @@ function createSequenceConverter(converter) { | |||
const val = converter(res.value, { | |||
__proto__: null, | |||
...opts, | |||
context: `${opts.context}, index ${array.length}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message change is not semver-major nor minor, as there is only one place using this converter and it's not using the context
, i.e. the error message is not even displayable in the userland.
8f1cafc
to
ad5664f
Compare
I am marking this as
semver-major
|
I agree this would be a semver-major. Does it mean a CITGM is required?
Would it be better to remove this change? This is unrelated to the issue that the PR is fixing and it can be avoided. |
Is there any documentation for supporting third party abortcontrollers? If not I could see this landing as a bug fix. |
There is only one benchmark test coverring @anonrig In that case, would it be better if I raise another PR to add benchmark tests to cover |
ad5664f
to
82da5e9
Compare
@nodejs/tsc per nodejs/Release#1034 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not brand checking AbortSignal was an intentional design choice in order to enable userland to migrate to using Node's AbortSignal.
We can choose to break that compatibility but it would have to be semver-major and require @nodejs/tsc's opinion since it's an ecosystem concern (this is likely to break userland code)
We made the same decision in undici to drop third party AbortController/AbortSignal support. |
Thinking about this more userland AbortController libraries have tens of millions of weekly downloads so I'm -0.5 (not for this change but still not blocking)
When did it happen (assuming it affects Node's |
I am -1 for this change. |
v7, which is releasing soon-ish. Breakage is unimportant here - it was undocumented behavior that broke compatibility with other implementations and the spec. If we implement a spec, we need to accept its burden too. If there are/were performance concerns, those need to be fixed in node core, not handed off to user land to create crappy (not to be offensive) abortcontroller implementations to be used instead. |
Would like share some of my humble opinions here.
I think the decision to make is whether or not we want to phase in brand check completely, i.e. drop support to 3rd party libs, or leaving it half-half is acceptable? |
FWIW the "breakability" of some libraries can be tested with a CITGM |
This PR contains 2 commits.
The first one implements interface converter as per the spec.
The one after uses the implemented interface converter to properly validate
AbortSiganl
s inAbortSignal.any
Fixes: #54962