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: mark callback-based fs API as legacy #37948

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Mar 27, 2021

A few folks have voiced support for bringing new features only to the Promise-based and sync APIs of fs in #37944. This PR marks the callback-API as "Legacy" to reflect this position.

@nodejs/fs wdyt?


For reference, "Legacy" status is defined by

> Stability: 3 - Legacy. The feature is no longer recommended for use. While it
> likely will not be removed, and is still covered by semantic-versioning
> guarantees, use of the feature should be avoided.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Mar 27, 2021
@mscdex
Copy link
Contributor

mscdex commented Mar 27, 2021

-1 I think this just adds unnecessary confusion, especially when it's only being applied for a single core module.

@jasnell
Copy link
Member

jasnell commented Mar 27, 2021

I disagree with it adding confusion. It is a legacy api that is unlikely to evolve significantly. Marking it as legacy makes that clearer.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

🌶️

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

This is quite a position change, while I agree with the idea conceptually - this definitely need TSC eyes @nodejs/tsc

@benjamingr benjamingr added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Mar 28, 2021
Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

+1 because new code should prefer using the promises based API instead of callback API.

Callback API is often not even used directly but is wrapped into something, also requires additional code or/and more complex structure which is more likely to contain bugs.

This change might give an (small) incentive to use the cleaner (as in: less prone to issues) API instead.

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.

I do not agree to use the legacy classification for these.

@addaleax
Copy link
Member

@mcollina Could you give an explanation?

@mcollina
Copy link
Member

I think that mixing errorbacks and promises is a source of bugs, crashes and leaks.

Marking fs errobacks methods as legacy is encouraging such mixed use in all those cases were a promise equivalent is not available.

I agree with the intent but we are not ready as there are still quite a few APIs that lack a promise equivalent both in Node.js and the ecosystem. As an example, all of our network stack is not compatible.

I think we should aim to mark all of the errorback methods as legacy in one single step.

@mscdex
Copy link
Contributor

mscdex commented Mar 28, 2021

Additionally, with all of the issues (that we haven't encountered with callbacks) I've seen surrounding Promises (e.g. behavior with unhandled, unresolved, etc.) and with Promises able to incur measurable overhead compared to callbacks (last I saw), I think it's a bad idea to label these as legacy.

@benjamingr
Copy link
Member

@mscdex

Additionally, with all of the issues (that we haven't encountered with callbacks) I've seen surrounding Promises

These issues have largely have already been worked on and have been solved. I see Matteo's point regarding networking though.

@mscdex
Copy link
Contributor

mscdex commented Mar 29, 2021

These issues have largely have already been worked on and have been solved. I see Matteo's point regarding networking though.

My point is until Promises and their behaviors have been stable for a substantial amount of time, we shouldn't be promoting them as the official/recommended/non-legacy/etc. mechanism for async APIs.

@tniessen
Copy link
Member

tniessen commented Apr 8, 2021

For reference, "Legacy" status is defined by

> Stability: 3 - Legacy. The feature is no longer recommended for use. While it
> likely will not be removed, and is still covered by semantic-versioning
> guarantees, use of the feature should be avoided.

Personally, I think the disagreement here boils down to the interpretation of the definition.

The definition says "The feature is no longer recommended for use."

But, as @mcollina said, in this case, it is probably closer to "The feature is no longer recommended for use... unless you are using callbacks in related parts of your application anyway, as to avoid mixing callbacks and Promises."

@mhdawson
Copy link
Member

mhdawson commented Apr 8, 2021

I personally agree that it's too early to be marking individual APIs like this as legacy.

@Flarna
Copy link
Member

Flarna commented Apr 9, 2021

Besides network APIs there is also crypto which uses callbacks only and a few new were added recently (refs: #37500).
How should we deal with PRs like #37500 in future if callbacks are considered legacy?

@mhdawson
Copy link
Member

mhdawson commented Apr 15, 2021

From the discussion in the TSC, the general feeling was

  1. It's too early to mark them as legacy
  2. A good alternative would be to update docs to discourage their use/explain the issue and how the promises based APIs solve them.

Taking off the TSC agenda, please re-add if there is another question/feedback needed.

@aduh95 aduh95 removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 15, 2021
@jasnell
Copy link
Member

jasnell commented Apr 28, 2021

Given the TSC discussion, closing this PR

@jasnell jasnell closed this Apr 28, 2021
@aduh95 aduh95 deleted the fs-legacy-callback branch April 28, 2021 22:12
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. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.