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

chore: switch to platform AbortController & Signal implementations after dropping Node.js 14 #1308

Merged
merged 10 commits into from
Jun 18, 2024

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Jun 14, 2024

fixes aws/aws-sdk-js-v3#4872
fixes #974

this deprecates our own Abort types and implementation and switches to the platform types where available.

AbortController / AbortSignal are available in Node.js 16+.

@kuhe kuhe requested review from a team as code owners June 14, 2024 20:54
@kuhe kuhe requested a review from sugmanue June 14, 2024 20:54
Copy link
Contributor

@trivikr trivikr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: removing redundant names in docs

packages/types/src/abort.ts Outdated Show resolved Hide resolved
packages/types/src/abort.ts Outdated Show resolved Hide resolved
@trivikr
Copy link
Contributor

trivikr commented Jun 14, 2024

Verified that AbortController was made stable in Node.js v15.4.0

https://nodejs.org/api/globals.html#class-abortcontroller

Copy link
Contributor

@syall syall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The jest fit needs to be reverted.

Is there a reason for the addEventListener() and .onabort branching? Is it to differentiate the platform` and custom controller implementation we had?

It'd also be nice to import every reference of the deprecated AbortController as `DeprecatedAbortController as you did in some files already.

packages/fetch-http-handler/src/fetch-http-handler.ts Outdated Show resolved Hide resolved
packages/node-http-handler/src/node-http-handler.spec.ts Outdated Show resolved Hide resolved
packages/node-http-handler/src/node-http-handler.ts Outdated Show resolved Hide resolved
packages/node-http-handler/src/node-http2-handler.ts Outdated Show resolved Hide resolved
Comment on lines +6 to +8
export interface AbortHandler {
(this: AbortSignal | DeprecatedAbortSignal, ev: any): any;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit question: will this ever need to be scoped down or have extra arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be ev: Event -> void, but I wouldn't change it at this point for compatibility

packages/util-waiter/src/createWaiter.ts Outdated Show resolved Hide resolved
@kuhe
Copy link
Contributor Author

kuhe commented Jun 18, 2024

confirmed TS22 is action runner memory issue

@kuhe kuhe merged commit c2a5595 into smithy-lang:main Jun 18, 2024
10 checks passed
@kuhe kuhe deleted the chore/abort branch June 18, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants