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

doc: remove --experimental-abortcontroller documentation #38968

Conversation

iam-frankqiu
Copy link
Contributor

According to the docs.

AbortController and AbortSignal support is enabled by default. So I think we should delete this option.

@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 8, 2021
@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 8, 2021
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Deleting a command-line flag is semver-major. It's marked as a non-op, which is appropriate for now. Let's hold off removing it for a while.

@addaleax
Copy link
Member

addaleax commented Jun 8, 2021

Fwiw, I think removing this from the docs for the current version is fine, but we’re not really getting anything out of removing the flag instead of having it be a no-op.

@iam-frankqiu
Copy link
Contributor Author

Deleting a command-line flag is semver-major. It's marked as a non-op, which is appropriate for now. Let's hold off removing it for a while.

Is it the right time to remove it now?

@addaleax
Copy link
Member

@iam-frankqiu At the risk of repeating what I’ve said before – I don’t think there’s any reason to remove the flag from the source code at any point in time. However, if you want to remove the flag from the documentation, I think we can do that right now.

@iam-frankqiu
Copy link
Contributor Author

@iam-frankqiu At the risk of repeating what I’ve said before – I don’t think there’s any reason to remove the flag from the source code at any point in time. However, if you want to remove the flag from the documentation, I think we can do that right now.

Thank you. What I want to do is just make node.js simpler and cleaner.

@iam-frankqiu iam-frankqiu added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 21, 2021
@dnalborczyk
Copy link
Contributor

dnalborczyk commented Oct 23, 2021

any reason to remove the flag from the source code at any point in time

@addaleax I think that something flagged as experimental should include the flag itself as being experimental, meaning it might just disappear at any given time. I really hope the flag is not being carried around forever, this makes no sense.

speaking of experimental: I get the feeling that the notion of experimental has lost its meaning in the node.js project - meaning most the time it is being treated as untouchable and semver major.

that's not what I understand of "experimenting". experimenting means I can break things, hence we flag it. "be aware! it might (and will) break!" but I think people have gotten used to the fact that node.js experimental stuff does not break. when I run any given project of mine I get more or less experimental warnings of some 3rd party modules using node.js built-ins.

if an application developer is using an experimental flag, I'm pretty sure it's easy to remove, and was set with the same mindset of being eventually removed. heck, most (if not all) of the companies don't even want you to use any "experimental" flags in production. because the word alone sounds brittle and unstable.

@aduh95 aduh95 changed the title refactor: delete experimental_abortcontroller flag doc: remove --experimental-abortcontroller documentation Oct 23, 2021
@aduh95
Copy link
Contributor

aduh95 commented Oct 23, 2021

speaking of experimental: I get the feeling that the notion of experimental has lost its meaning in the node.js project - meaning most the time it is being treated as untouchable and semver major.

I can think of several recent changes to some Node.js experimental API changes, but let's not debate on this. I think Anna's point is:

  1. it's not because it's experimental that it has to break.
  2. keeping a no-op flag costs virtually nothing, removing it is a breaking change with no benefit.

Consider this: keeping the flag allows users to use globalThis.AbortController on Node.js 14.x while keeping full compatibility with Node.js 16.x+. Removing it would make this impossible (or rather needlessly over complicated). If you feel strongly, consider opening another PR when Node.js 14.x has reached EOL.

@iam-frankqiu iam-frankqiu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 24, 2021
@addaleax
Copy link
Member

To add on to what @aduh95 said –

most (if not all) of the companies don't even want you to use any "experimental" flags in production. because the word alone sounds brittle and unstable

Great, then it sounds like this is working as it should?

@addaleax
Copy link
Member

Also, for some more context, we specifically put this into our Collaborator guide to avoid having this discussion over and over again:

Avoid Runtime Deprecations when an alias or a stub/no-op will suffice. An alias or stub will have lower maintenance costs for end users and Node.js core.

@iam-frankqiu iam-frankqiu force-pushed the del_experimental_abortcontroller branch from 6c11735 to 277146b Compare October 27, 2021 14:06
@iam-frankqiu iam-frankqiu added doc Issues and PRs related to the documentations. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Oct 27, 2021
@aduh95
Copy link
Contributor

aduh95 commented Nov 24, 2021

@jasnell are you still -1 on this now that the change is doc-only?

jasnell pushed a commit that referenced this pull request Nov 25, 2021
PR-URL: #38968
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@jasnell
Copy link
Member

jasnell commented Nov 25, 2021

Landed in 24292b4

@jasnell jasnell closed this Nov 25, 2021
targos pushed a commit that referenced this pull request Nov 26, 2021
PR-URL: #38968
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
PR-URL: #38968
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #38968
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants