fs: remove experimental warning for fs.promises by addaleax · Pull Request #26581 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation19 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

addaleax

This has been warning for long enough, without any API changes
in the last few months.

Checklist

bricss, PazzaVlad, Jessidhia, anton-bot, and tomap reacted with thumbs up emoji lundibundi, nechaido, benjamingr, devsnek, bricss, antsmartian, shisama, yaelhe, aduh95, ZYSzys, and 6 more reacted with hooray emoji anton-bot reacted with heart emoji anton-bot reacted with rocket emoji

@nodejs-github-bot

lpinca

@devsnek

we still have the unsolved issue of it needing to be first class accessable to esm

@addaleax

@devsnek Is that a blocker for you for this PR?

@devsnek

@addaleax I think so. would it be unreasonable to move this back to fs/promises or at least add that as an alias?

benjamingr

@benjamingr

I think so. would it be unreasonable to move this back to fs/promises or at least add that as an alias?

That is blocked on scoping Node's modules, so it would need to be @nodejs/fs or something similar. That's why fs/promises was reverted to begin with.

@devsnek

@benjamingr then it sounds like we should figure out the rest of the scoping

cjihrig

@Trott Trott added semver-major

PRs that contain breaking changes and should be released in the next major version.

and removed semver-major

PRs that contain breaking changes and should be released in the next major version.

labels

Mar 11, 2019

@Trott

This leaves it as experimental in the docs. That's intentional? (I'd prefer we make it stable in the docs and label this semver-major but that's just me!)

@jasnell

Perhaps it's time just to take it out of experimental?

antsmartian

Choose a reason for hiding this comment

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

🎉

jdalton

@@ -1927,11 +1927,8 @@ Object.defineProperties(fs, {
configurable: true,
enumerable: false,
get() {
if (promises === null) {

Choose a reason for hiding this comment

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

Please make this property enumerable: true now that it's no longer experimental.
It also no longer needs to be a getter.

Choose a reason for hiding this comment

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

@addaleax

@jasnell @Trott I’m also okay with completely moving it out of experimental status, yes.

I’m not sure about the scoping thing. It doesn’t feel to me like we should block features until we’ve figured that out, it’s been a long time and there hasn’t been much movement on it recently.

@BridgeAR BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Mar 25, 2019

@nodejs-github-bot

@BridgeAR

I marked this as author ready. It seems we can still revise the actual experimental status in another PR and that should not block the message from being removed.

@targos

The tests failed everywhere.

@addaleax

This has been warning for long enough, without any API changes in the last few months.

@addaleax

@Trott Trott removed the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Mar 30, 2019

@Trott

(Removed author ready because of failing test, but feel free to re-add, as I suspect the test fix is likely to be something small and straightforward.)

@addaleax

@addaleax

@targos @Trott Yes, thanks – this should be fixed now.

I’ve also aligned this PR with what #26592 did, i.e. moved the API to Stable rather than just removing the warning.

BethGriggs pushed a commit that referenced this pull request

Apr 9, 2019

@addaleax @BethGriggs

This has been warning for long enough, without any API changes in the last few months.

PR-URL: #26581 Reviewed-By: Luigi Pinca luigipinca@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Anto Aravinth anto.aravinth.cse@gmail.com Reviewed-By: Yongsheng Zhang zyszys98@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Signed-off-by: Beth Griggs Bethany.Griggs@uk.ibm.com

BethGriggs pushed a commit that referenced this pull request

Apr 10, 2019

@addaleax @BethGriggs

This has been warning for long enough, without any API changes in the last few months.

PR-URL: #26581 Reviewed-By: Luigi Pinca luigipinca@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Anto Aravinth anto.aravinth.cse@gmail.com Reviewed-By: Yongsheng Zhang zyszys98@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Signed-off-by: Beth Griggs Bethany.Griggs@uk.ibm.com

BethGriggs added a commit that referenced this pull request

Apr 11, 2019

@BethGriggs

Notable changes:

PR-URL: #27163

BethGriggs added a commit that referenced this pull request

Apr 11, 2019

@BethGriggs

Notable changes:

PR-URL: #27163

BethGriggs pushed a commit that referenced this pull request

Oct 16, 2019

@addaleax @BethGriggs

This has been warning for long enough, without any API changes in the last few months.

PR-URL: #26581 Reviewed-By: Luigi Pinca luigipinca@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Anto Aravinth anto.aravinth.cse@gmail.com Reviewed-By: Yongsheng Zhang zyszys98@gmail.com Reviewed-By: James M Snell jasnell@gmail.com

BethGriggs added a commit that referenced this pull request

Oct 18, 2019

@BethGriggs

Notable changes:

PR-URL: #29875

BethGriggs added a commit that referenced this pull request

Oct 19, 2019

@BethGriggs

Notable changes:

PR-URL: #29875

BethGriggs added a commit that referenced this pull request

Oct 21, 2019

@BethGriggs

Notable changes:

PR-URL: #29875

BethGriggs added a commit that referenced this pull request

Oct 22, 2019

@BethGriggs

Notable changes:

PR-URL: #29875

BethGriggs added a commit that referenced this pull request

Oct 22, 2019

@BethGriggs

Notable changes:

PR-URL: #29875

@iiroj iiroj mentioned this pull request

Jan 31, 2020

devtools-bot pushed a commit to ChromeDevTools/devtools-frontend that referenced this pull request

Sep 4, 2020

babot pushed a commit to binaryage/dirac that referenced this pull request

Sep 4, 2020

Reviewers

@jdalton jdalton jdalton left review comments

@jasnell jasnell jasnell approved these changes

@antsmartian antsmartian antsmartian approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@ZYSzys ZYSzys ZYSzys approved these changes

Labels

author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

fs

Issues and PRs related to the fs subsystem / file system.

semver-minor

PRs that contain new features and should be released in the next minor version.