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.

Sign up for GitHub

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

Jump to bottom

doc,lib: remove deprecated domain module #45839

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

doc,lib: remove deprecated domain module #45839

wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Dec 13, 2022 •
edited
Loading

Fixes: #45824

Draft PR to check citgm. The REPL was leaning quite heavily on domains so REPL tests are expected to fail. I didn't want to spend time on that for a trial balloon.

I deleted lib/domain.js for now but in the final version it should probably throw an exception (not emit a warning.)

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3060/
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/startup
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Dec 13, 2022
aduh95
aduh95 reviewed Dec 13, 2022
View reviewed changes
doc/api/deprecations.md
description: Documentation-only deprecation.
-->

Type: Documentation-only
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to first runtime deprecate it.
Copy link
Member Author

Choose a reason for hiding this comment

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

Note my "draft PR to check citgm" remark (which is inconclusive so far - citgm seems to be in bad shape)

That said... the domain module has basically always been deprecated0. Standard procedures are nice but sometimes it makes no sense to follow them.

0 it was added in v0.10.0 and deprecated in v1.0.0. We'll not talk about the brown paper bag releases that were v0.12.x.
doc/api/deprecations.md
@@ -717,24 +717,6 @@ Type: Documentation-only
The [`ecdh.setPublicKey()`][] method is now deprecated as its inclusion in the
API is not useful.

### DEP0032: `node:domain` module
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to keep this entry, it should say Type: End-of-Life, e.g. see

node/doc/api/deprecations.md

Lines 44 to 64 in 4712d60

### DEP0001: `http.OutgoingMessage.prototype.flush`
<!-- YAML
changes:
- version: v14.0.0
pr-url: https://github.com/nodejs/node/pull/31164
description: End-of-Life.
- version:
- v6.12.0
- v4.8.6
pr-url: https://github.com/nodejs/node/pull/10116
description: A deprecation code has been assigned.
- version: v1.6.0
pr-url: https://github.com/nodejs/node/pull/1156
description: Runtime deprecation.
-->
Type: End-of-Life
`OutgoingMessage.prototype.flush()` has been removed. Use
`OutgoingMessage.prototype.flushHeaders()` instead.

doc/api/errors.md
Comment on lines -1216 to -1233
<a id="ERR_DOMAIN_CALLBACK_NOT_AVAILABLE"></a>

### `ERR_DOMAIN_CALLBACK_NOT_AVAILABLE`

The `node:domain` module was not usable since it could not establish the
required error handling hooks, because
[`process.setUncaughtExceptionCaptureCallback()`][] had been called at an
earlier point in time.

<a id="ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE"></a>

### `ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`

[`process.setUncaughtExceptionCaptureCallback()`][] could not be called
because the `node:domain` module has been loaded at an earlier point in time.

The stack trace is extended to include the point in time at which the
`node:domain` module had been loaded.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be moved to https://nodejs.org/api/errors.html#legacy-nodejs-error-codes instead of being removed.
@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 13, 2022
@Trott
Copy link
Member

Trott commented Dec 14, 2022 •
edited
Loading

Draft PR to check citgm.

I might be telling you something you already know, but just in case: You can open a PR like this against your own fork and then put bnoordhuis rather than nodejs as the GITHUB_ORG in the Jenkins build parameter. The upside is you avoid alarming people or attracting PR reviews. The downside is that the results won't be documented in a PR in the main repo, so I'm fine with doing it this way instead.

EDIT: Added some additional labels to hopefully further dissuade people from thinking this is ready for review. Feel free to remove them at any time.
@Trott Trott added wip Issues and PRs that are still a work in progress. blocked PRs that are blocked by other issues or PRs. labels Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version. wip Issues and PRs that are still a work in progress.
4 participants
@bnoordhuis @nodejs-github-bot @Trott @aduh95