Skip to content

events: disabled manual construction AbortSignal - #36094

Closed
RaisinTen wants to merge 1 commit into
nodejs:masterfrom
RaisinTen:stop-manual-construction-AbortController
Closed

events: disabled manual construction AbortSignal#36094
RaisinTen wants to merge 1 commit into
nodejs:masterfrom
RaisinTen:stop-manual-construction-AbortController

Conversation

@RaisinTen

@RaisinTenRaisinTen commented Nov 12, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Fixes: #36064

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@RaisinTen
RaisinTenforce-pushed the stop-manual-construction-AbortController branch 2 times, most recently from 61e66b6 to d6a9e3eCompareNovember 12, 2020 14:35
@RaisinTen
RaisinTenforce-pushed the stop-manual-construction-AbortController branch 3 times, most recently from d353925 to 99dd10bCompareNovember 13, 2020 17:12
@RaisinTen
RaisinTen marked this pull request as ready for review November 13, 2020 18:19
@benjamingrbenjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 13, 2020
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 13, 2020
@nodejs-github-bot

nodejs-github-bot commented Nov 13, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34384/

@nodejs-github-bot

nodejs-github-bot commented Nov 13, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34385/

@aduh95aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 13, 2020
aduh95
aduh95 reviewed Nov 13, 2020
View reviewed changes
Comment threadlib/internal/abort_controller.js Outdated

@aduh95aduh95Nov 13, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why is this comment needed?

@TrottTrottNov 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of curiosity, why is this comment needed?

It's because of this:

node/lib/.eslintrc.yaml

Lines 22 to 23 in c7d2a45

- selector: "NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError)$/])"
message: "Use an error exported by the internal/errors module."
.

Trott
Trott reviewed Nov 14, 2020
View reviewed changes
Comment threadlib/internal/abort_controller.js Outdated
@RaisinTen
RaisinTenforce-pushed the stop-manual-construction-AbortController branch from 99dd10b to ef80268CompareNovember 14, 2020 05:22
@lpincalpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 14, 2020
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 14, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Nov 14, 2020•
edited by aduh95
Loading

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34390/✅

@aduh95aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 14, 2020
@github-actionsgithub-actionsBot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 14, 2020
@github-actions

github-actionsBot commented Nov 14, 2020

Copy link
Copy Markdown
Contributor

Landed in 1d02a35...22293ea

nodejs-github-bot pushed a commit that referenced this pull request Nov 14, 2020
@RaisinTen@nodejs-github-bot
events: disabled manual construction AbortSignal
22293ea
Fixes: #36064
PR-URL: #36094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@RaisinTen
RaisinTen deleted the stop-manual-construction-AbortController branch November 14, 2020 14:53
codebytere pushed a commit that referenced this pull request Nov 22, 2020
@RaisinTen@codebytere
events: disabled manual construction AbortSignal
1550073
Fixes: #36064
PR-URL: #36094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebyterecodebytere mentioned this pull request Nov 22, 2020
Merged
@targostargos mentioned this pull request Apr 24, 2021
Closed
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
@RaisinTen@targos
events: disabled manual construction AbortSignal
0131586
Fixes: nodejs#36064
PR-URL: nodejs#36094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@RaisinTen@targos
events: disabled manual construction AbortSignal
64cd54b
Fixes: #36064
PR-URL: #36094
Backport-PR-URL: #38386
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targostargos added backported-to-v14.x events Issues and PRs related to the events subsystem / EventEmitter. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v14.x labels Apr 30, 2021
@danielleadamsdanielleadams mentioned this pull request May 3, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95aduh95aduh95 left review comments
@jasnelljasnelljasnell approved these changes
@TrottTrottTrott approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
@lpincalpincalpinca approved these changes
@himself65himself65himself65 approved these changes
+1 more reviewer
@puzpuzpuzpuzpuzpuzpuzpuzpuz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

eventsIssues and PRs related to the events subsystem / EventEmitter.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

It should be impossible to construct an AbortSignal manually

10 participants

@RaisinTen@nodejs-github-bot@jasnell@Trott@benjamingr@lpinca@himself65@aduh95@puzpuzpuz@targos