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

err: document ABORT_ERR code #36319

Closed

Conversation

benjamingr
Copy link
Member

We added an AbortError with the same code and name as the web's as
part of the consensus in #36084 but
never actually documented the error in our error codes list.

This PR adds the error code.

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

@benjamingr benjamingr added the doc Issues and PRs related to the documentations. label Nov 29, 2020
@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Nov 29, 2020
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 1, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 1, 2020
@github-actions

This comment has been minimized.

@github-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Dec 1, 2020
@benjamingr benjamingr added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 1, 2020
@github-actions github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 1, 2020
@github-actions

This comment has been minimized.

@benjamingr benjamingr added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 1, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 1, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@@ -612,6 +612,16 @@ A human-readable string describing the reason for the error.
<a id="nodejs-error-codes"></a>
## Node.js error codes

<a id="ERR_ABORT"></a>
### `ABRT_ERR`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### `ABRT_ERR`
### `ABRT_ERR`
<!-- YAML
added: v15.0.0
-->

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically it's not 15.0 and this code existed on DOMException beforehand and users could craft it. The actual non DOMException AbortError is 15.2 - I am not sure what this should say?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should say whatever is the first version in which Node.js actually throws errors with this code.

Copy link
Member Author

Choose a reason for hiding this comment

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

What version first had DOMException? 8?

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused about this code. Could you point me to where it is defined? I cannot find any occurence of ABRT_ERR in the source code.

Copy link
Member Author

Choose a reason for hiding this comment

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

@targos wow, we all missed the typo - it's ABORT_ERR and not ABRT_ERR.

I'll push a fix, the error is defined in two places - in internal/errors.j (as class AbortError extends Error {) and in lib/internal/per_context/domexception.js.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't even think it could be a typo!

So I checked and v15.0.0 is indeed the first version where we throw errors with that code. I don't think we should mention versions where users could somehow do it themselves.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

We added an `AbortError` with the same code and name as the web's as
part of the consensus in nodejs#36084 but
never actually documented the error in our error codes list.

This PR adds the error code.
@benjamingr
Copy link
Member Author

I think this probably needed a rebase 🤔

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

@benjamingr
Copy link
Member Author

Landed in a2a2ff3

@benjamingr benjamingr closed this Dec 9, 2020
@Trott
Copy link
Member

Trott commented Dec 9, 2020

Landed in a2a2ff3

This commit was added to master without metadata. Not sure there's much to do at this point other than notify @nodejs/releasers so they can work around it in releases. (Actually, I'd be OK with a force push to fix if there was consensus on @nodejs/tsc. It's been 6 hours so far....)

@gireeshpunathil
Copy link
Member

+1 to force-push.
(curious to know how did you catch this?)

@Trott
Copy link
Member

Trott commented Dec 9, 2020

(curious to know how did you catch this?)

When I rebase against master, I scan the new commits to see what's been landed. I never use the bring-master-up-to-date feature in node-core-utils and always do it manually before landing anything specifically so I can do this quick scan.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Belated LGTM

@Trott
Copy link
Member

Trott commented Dec 9, 2020

In addition to no metadata, it also has an invalid subsystem (err)`. Force pushing momentarily unless someone swoops in here or in IRC to say otherwise.

Trott pushed a commit that referenced this pull request Dec 9, 2020
We added an `AbortError` with the same code and name as the web's as
part of the consensus in #36084 but
never actually documented the error in our error codes list.

This PR adds the error code.

PR-URL: #36319
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Trott
Copy link
Member

Trott commented Dec 9, 2020

Force pushed to fix metadata and subsystem stuff.

Landed in 13e2170

@benjamingr
Copy link
Member Author

@Trott thanks, what did I do wrong?

benjamingruenbaum@Benjamins-MBP node % git node land 36319
✔  Done loading data for nodejs/node/pull/36319
----------------------------------- PR info ------------------------------------
Title      err: document ABORT_ERR code (#36319)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     benjamingr:document-abort-error-code -> nodejs:master
Labels     doc, errors
Commits    1
 - err: document ABORT_ERR code
Committers 1
 - Benjamin Gruenbaum <benjamingr@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36319
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - err: document ABORT_ERR code
   ℹ  Last Full PR CI on 2020-12-08T22:12:01Z: https://ci.nodejs.org/job/node-test-pull-request/34862/
✔  Build data downloaded
   ℹ  This PR was created on Sun, 29 Nov 2020 18:13:42 GMT
   ✔  Approvals: 4
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/36319#pullrequestreview-540505050
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/36319#pullrequestreview-540511955
   ✔  - Daijiro Wachi (@watilde): https://github.com/nodejs/node/pull/36319#pullrequestreview-540839835
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/36319#pullrequestreview-542270150
--------------------------------------------------------------------------------
? This PR is not ready to land, do you want to continue? Yes
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
? Do you want to try reset the local master branch to origin/master? Yes
⠙ Bringing origin/master up to date...From github.com:nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
✔  Downloaded patch to /Users/benjamingruenbaum/Documents/Projects/node/.ncu/36319/patch
--------------------------------------------------------------------------------
Applying: err: document ABORT_ERR code
   ✔  Patches applied
--------------------------------------------------------------------------------
? There is only one commit in this PR.
do you want to amend the commit message? No

(Next commands are: I rant lint & tests and then pushed)

@targos
Copy link
Member

targos commented Dec 9, 2020

do you want to amend the commit message? No

You have to answer "Yes" to this question, otherwise the commit message is not amended, so the metadata is not added to it

About the subsystem, I thought it was validated by git node land but maybe I don't remember correctly.

@benjamingr
Copy link
Member Author

@targos

You have to answer "Yes" to this question, otherwise the commit message is not amended, so the metadata is not added to it

Thanks, my bad. I thought that it's not possible to land stuff with git node land without metadata.

I'll just check the commit log before pushing the next few times in case there is more stuff I miss.

targos pushed a commit that referenced this pull request Dec 21, 2020
We added an `AbortError` with the same code and name as the web's as
part of the consensus in #36084 but
never actually documented the error in our error codes list.

This PR adds the error code.

PR-URL: #36319
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request May 16, 2021
We added an `AbortError` with the same code and name as the web's as
part of the consensus in #36084 but
never actually documented the error in our error codes list.

This PR adds the error code.

PR-URL: #36319
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Jun 11, 2021
We added an `AbortError` with the same code and name as the web's as
part of the consensus in #36084 but
never actually documented the error in our error codes list.

This PR adds the error code.

PR-URL: #36319
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants