Skip to content

util: move deprecated utils to runtime deprecation#50488

Closed
marco-ippolito wants to merge 18 commits into
nodejs:mainfrom
marco-ippolito:runtime-deprecation
Closed

util: move deprecated utils to runtime deprecation#50488
marco-ippolito wants to merge 18 commits into
nodejs:mainfrom
marco-ippolito:runtime-deprecation

Conversation

@marco-ippolito

@marco-ippolito marco-ippolito commented Oct 31, 2023

Copy link
Copy Markdown
Member

The deprecation code has been created in 6.12.0, I think we can move to runtime deprecation in 22
Moving the following APIs from documentation-only deprecation to runtime deprecation:

  • util._extend
  • util.isArray
  • util.isBoolean
  • util.isBuffer
  • util.isDate
  • util.isError
  • util.isFunction
  • util.isNull
  • util.isNullOrUndefined
  • util.isNumber
  • util.isObject
  • util.isPrimitive
  • util.isRegExp
  • util.isString
  • util.isSymbol
  • util.isUndefined
  • util.log

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Oct 31, 2023
@marco-ippolito marco-ippolito added the deprecations Issues and PRs related to deprecations. label Oct 31, 2023
Comment thread doc/api/deprecations.md Outdated
Comment thread doc/api/deprecations.md
@aduh95

aduh95 commented Oct 31, 2023

Copy link
Copy Markdown
Contributor

Is there any particular reason to do so? If not, we might as well keep it as a doc-only deprecation.

@marco-ippolito

marco-ippolito commented Oct 31, 2023

Copy link
Copy Markdown
Member Author

Not a particular reason per se but if it was deprecated it means it somehow needs to be removed, if we dont want to remove it, why deprecate it? It's dead code, we can probably runtime deprecate it in 22 and drop in 23.
And remove a part of documentation in utils called 'deprecated APIs' 😆

@aduh95

aduh95 commented Oct 31, 2023

Copy link
Copy Markdown
Contributor

why deprecate it?

Because it fits one of those cases:

Node.js APIs might be deprecated for any of the following reasons:
* Use of the API is unsafe.
* An improved alternative API is available.
* Breaking changes to the API are expected in a future major release.

Being doc-deprecated doesn't mean it has to ever be runtime deprecated:

cycle. There is no rule that deprecated code must progress to End-of-Life.
Documentation-Only and Runtime Deprecations can remain in place for an unlimited
duration.

@marco-ippolito

marco-ippolito commented Oct 31, 2023

Copy link
Copy Markdown
Member Author

I feel like that's more of the case of features like Domain which are probably gonna stay there forever.
Might want to add this to TSC agenda?

@marco-ippolito marco-ippolito added semver-major PRs that contain breaking changes and should be released in the next major version. notable-change PRs with changes that should be highlighted in changelogs. labels Oct 31, 2023
@github-actions

Copy link
Copy Markdown
Contributor

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @marco-ippolito.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

@aduh95

aduh95 commented Oct 31, 2023

Copy link
Copy Markdown
Contributor

Anyone can add (or request to add) the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label, but at this step, there's not much for the TSC to discuss, it's up to the collaborators to accept or reject this PR. (well because it is semver-major PRs that contain breaking changes and should be released in the next major version. , it also needs approval from at least 2 TSC voting members, but still the TSC doesn't need to weigh in unless collaborators fail to find consensus).

@RafaelGSS RafaelGSS left a comment

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.

SGTM

@RafaelGSS

Copy link
Copy Markdown
Member

@marco-ippolito please update the PR description to include the list of functions that will be moved from doc-deprecation to runtime deprecation.

@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 1, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 1, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@aduh95

aduh95 commented Nov 1, 2023

Copy link
Copy Markdown
Contributor

I think this is adding unnecessary pain to our users for very little benefit for us (if any at all). Anyway, ping @nodejs/tsc since this is semver major.

@ronag ronag left a comment

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.

I don't see any benefit of this and a lot of annoyance. IMHO. We should only runtime deprecate things that we eventually intend to remove. There is small to no maintenance overhead on these so I don't see why we would.

@marco-ippolito

marco-ippolito commented Nov 1, 2023

Copy link
Copy Markdown
Member Author

Most of these API have been deprecated since v4, almost 9 years ago.
We have litteraly a section in our documentation just for these API

function isString(arg) {
  return typeof arg === 'string';
}

It's moving to runtime deprecation so no work from package maintainers required.
I dont like the idea to keep those like in a museum, I would agree if it was something that actually had some utility, saw some use or the ecosystem is based on them (like Domain)

@aduh95

aduh95 commented Nov 1, 2023

Copy link
Copy Markdown
Contributor

I dont like the idea to keep those like in a museum

But why? Is it just out of principle? Are you on board with causing some (tiny) amount of pain to our users just for a question of principle? It seems to me we would need some other justification for that.

@targos

targos commented Nov 1, 2023

Copy link
Copy Markdown
Member

It's moving to runtime deprecation so no work from package maintainers required.

As soon as people start seeing a runtime deprecation, they will ask package maintainers to handle it.

I dont like the idea to keep those like in a museum

We're not keeping them like in a museum. We're keeping them to avoid "breaking" people for little benefit. That's also why the Buffer constructor is still not runtime-deprecated.

saw some use or the ecosystem is based on them

We can't really and don't measure use of features, so there's no reliable way to say whether the ecosystem is based on them.

@marco-ippolito

marco-ippolito commented Nov 1, 2023

Copy link
Copy Markdown
Member Author

We have already removed in the past some of these utils:
util.error, util.debug, util.puts, and more
for reference #25377 @cjihrig

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. deprecations Issues and PRs related to deprecations. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.