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

fs: remove experimental warning for fs.promises #26581

Closed

Conversation

@addaleax
Copy link
Member

commented Mar 11, 2019

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@lpinca

lpinca approved these changes Mar 11, 2019

@devsnek

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

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

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

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

@devsnek

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

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

@benjamingr

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

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

@cjihrig cjihrig referenced this pull request Mar 11, 2019

Merged

dns: remove dns.promises experimental warning #26592

3 of 3 tasks complete

@vsemozhetbyt vsemozhetbyt referenced this pull request Mar 11, 2019

Closed

Preparation for Node.js 12 #417

5 of 6 tasks complete

@Trott Trott added semver-major and removed semver-major labels Mar 11, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

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

@antsmartian
Copy link
Contributor

left a comment

🎉

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

This comment has been minimized.

Copy link
@jdalton

jdalton Mar 12, 2019

Member

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

This comment has been minimized.

Copy link
@addaleax

addaleax Mar 12, 2019

Author Member

@jdalton Thanks, done :)

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@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.

@nodejs-github-bot

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

The tests failed everywhere.

addaleax added some commits Mar 11, 2019

fs: remove experimental warning for fs.promises
This has been warning for long enough, without any API changes
in the last few months.

@Trott Trott removed the author ready label Mar 30, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

(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 force-pushed the addaleax:fs-promises-no-experimental-warning branch from 894643a to b9154d3 Mar 30, 2019

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2019

@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.

@nodejs-github-bot

This comment has been minimized.

@ZYSzys

ZYSzys approved these changes Mar 31, 2019

@ZYSzys ZYSzys added the author ready label Mar 31, 2019

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Landed in 5583d4d 🎉

@addaleax addaleax closed this Apr 2, 2019

@addaleax addaleax deleted the addaleax:fs-promises-no-experimental-warning branch Apr 2, 2019

addaleax added a commit that referenced this pull request Apr 2, 2019

fs: remove experimental warning for fs.promises
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 Apr 5, 2019

fs: remove experimental warning for fs.promises
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 Apr 9, 2019

fs: remove experimental warning for fs.promises
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 9, 2019

fs: remove experimental warning for fs.promises
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 BethGriggs referenced this pull request Apr 9, 2019

Merged

v11.14.0 proposal #27163

BethGriggs added a commit that referenced this pull request Apr 10, 2019

fs: remove experimental warning for fs.promises
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

2019-04-11, Version 11.14.0 (Current)
Notable changes:

- child_process: doc deprecate ChildProcess.\_channel (cjihrig)
  [#26982](#26982)
- deps: update nghttp2 to 1.37.0 (gengjiawen)
  [#26990](#26990)
- dns:
  - make dns.promises enumerable (cjihrig)
    [#26592](#26592)
  - remove dns.promises experimental warning (cjihrig)
    [#26592](#26592)
- fs: remove experimental warning for fs.promises (Anna Henningsen)
  [#26581] (#26581)
- stream: make Symbol.asyncIterator support stable (Matteo Collina)
  [#26989](#26989)
- worker: use copy of process.env (Anna Henningsen)
  [#26544](#26544)

PR-URL: #27163

BethGriggs added a commit that referenced this pull request Apr 11, 2019

2019-04-11, Version 11.14.0 (Current)
Notable changes:

- child_process: doc deprecate ChildProcess.\_channel (cjihrig)
  [#26982](#26982)
- deps: update nghttp2 to 1.37.0 (gengjiawen)
  [#26990](#26990)
- dns:
  - make dns.promises enumerable (cjihrig)
    [#26592](#26592)
  - remove dns.promises experimental warning (cjihrig)
    [#26592](#26592)
- fs: remove experimental warning for fs.promises (Anna Henningsen)
  [#26581] (#26581)
- stream: make Symbol.asyncIterator support stable (Matteo Collina)
  [#26989](#26989)
- worker: use copy of process.env (Anna Henningsen)
  [#26544](#26544)

PR-URL: #27163

@dnalborczyk dnalborczyk referenced this pull request Apr 15, 2019

Open

test: fs promises, fixes comment, simplifies test #27242

2 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.