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

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

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Mar 11, 2019
@devsnek
Copy link
Member

devsnek commented Mar 11, 2019

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

@addaleax
Copy link
Member Author

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

@devsnek
Copy link
Member

devsnek 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
Copy link
Member

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
Copy link
Member

devsnek commented Mar 11, 2019

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

@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
Copy link
Member

Trott 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
Copy link
Member

jasnell commented Mar 11, 2019

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

Copy link
Contributor

@antsmartian antsmartian left a comment

Choose a reason for hiding this comment

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

🎉

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

@jdalton jdalton Mar 12, 2019

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jdalton Thanks, done :)

@addaleax
Copy link
Member Author

@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
Copy link
Collaborator

@BridgeAR
Copy link
Member

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
Copy link
Member

targos commented Mar 30, 2019

The tests failed everywhere.

This has been warning for long enough, without any API changes
in the last few months.
@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
Copy link
Member

Trott 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 fs-promises-no-experimental-warning branch from 894643a to b9154d3 Compare March 30, 2019 21:50
@addaleax
Copy link
Member Author

@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
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 mentioned this pull request Apr 9, 2019
BethGriggs pushed a commit that referenced this pull request Apr 10, 2019
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
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
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 pushed a commit that referenced this pull request Oct 16, 2019
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
Notable changes:

- **deps**: upgrade openssl sources to 1.1.1d (Sam Roberts)
  [#29921](#29921)
- **dns**: remove dns.promises experimental warning (cjihrig)
  [#26592](#26592)
- **fs**: remove experimental warning for fs.promises (Anna Henningsen)
  [#26581](#26581)
- **n-api**: mark version 5 N-APIs as stable (Gabriel Schulhof)
  [#29401](#29401)
- **stream**: make Symbol.asyncIterator support stable (Matteo Collina)
  [#26989](#26989)

PR-URL: #29875
@BethGriggs BethGriggs mentioned this pull request Oct 18, 2019
BethGriggs added a commit that referenced this pull request Oct 19, 2019
Notable changes:

- **deps**: update npm to 6.11.3 (claudiahdz)
  [#29430](#29430)
- **deps**: upgrade openssl sources to 1.1.1d (Sam Roberts)
  [#29921](#29921)
- **dns**: remove dns.promises experimental warning (cjihrig)
  [#26592](#26592)
- **fs**: remove experimental warning for fs.promises (Anna Henningsen)
  [#26581](#26581)
- **n-api**: mark version 5 N-APIs as stable (Gabriel Schulhof)
  [#29401](#29401)
- **stream**: make Symbol.asyncIterator support stable (Matteo Collina)
  [#26989](#26989)

PR-URL: #29875
BethGriggs added a commit that referenced this pull request Oct 21, 2019
Notable changes:

- **deps**: update npm to 6.11.3 (claudiahdz)
  [#29430](#29430)
- **deps**: upgrade openssl sources to 1.1.1d (Sam Roberts)
  [#29921](#29921)
- **dns**: remove dns.promises experimental warning (cjihrig)
  [#26592](#26592)
- **fs**: remove experimental warning for fs.promises (Anna Henningsen)
  [#26581](#26581)
- **n-api**: mark version 5 N-APIs as stable (Gabriel Schulhof)
  [#29401](#29401)
- **stream**: make Symbol.asyncIterator support stable (Matteo Collina)
  [#26989](#26989)

PR-URL: #29875
BethGriggs added a commit that referenced this pull request Oct 22, 2019
Notable changes:

* crypto:
  * add support for chacha20-poly1305 for AEAD (chux0519)
    #24081
  * increase maxmem range from 32 to 53 bits (Tobias Nießen)
    #28799
* deps:
  * update npm to 6.11.3 (claudiahdz)
    #29430
  * upgrade openssl sources to 1.1.1d (Sam Roberts)
    #29921
* dns:
  * remove dns.promises experimental warning (cjihrig)
    #26592
* fs:
  * remove experimental warning for fs.promises (Anna Henningsen)
    #26581
* http:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* http2:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* n-api:
  * make func argument of napi\_create\_threadsafe\_function optional
    (legendecas)
    #27791
  * mark version 5 N-APIs as stable (Gabriel Schulhof)
    #29401
  * implement date object (Jarrod Connolly)
    #25917
* process:
  * add --unhandled-rejections flag (Ruben Bridgewater)
    #26599
* stream:
  * implement Readable.from async iterator utility (Guy Bedford)
    #27660
  * make Symbol.asyncIterator support stable (Matteo Collina)
    #26989

PR-URL: #29875
BethGriggs added a commit that referenced this pull request Oct 22, 2019
Notable changes:

* crypto:
  * add support for chacha20-poly1305 for AEAD (chux0519)
    #24081
  * increase maxmem range from 32 to 53 bits (Tobias Nießen)
    #28799
* deps:
  * update npm to 6.11.3 (claudiahdz)
    #29430
  * upgrade openssl sources to 1.1.1d (Sam Roberts)
    #29921
* dns:
  * remove dns.promises experimental warning (cjihrig)
    #26592
* fs:
  * remove experimental warning for fs.promises (Anna Henningsen)
    #26581
* http:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* http2:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* n-api:
  * make func argument of napi\_create\_threadsafe\_function optional
    (legendecas)
    #27791
  * mark version 5 N-APIs as stable (Gabriel Schulhof)
    #29401
  * implement date object (Jarrod Connolly)
    #25917
* process:
  * add --unhandled-rejections flag (Ruben Bridgewater)
    #26599
* stream:
  * implement Readable.from async iterator utility (Guy Bedford)
    #27660
  * make Symbol.asyncIterator support stable (Matteo Collina)
    #26989

PR-URL: #29875
devtools-bot pushed a commit to ChromeDevTools/devtools-frontend that referenced this pull request Sep 4, 2020
The current Loc presubmit are doing two checks: check_localizable_resources and check_localizability
(Details of what they are checking: https://docs.google.com/document/d/1L6TkT2-42MMQ72ZSBMFwUaq7M6mDgA2X0x8oHHKaV_U/edit#heading=h.w1no7qaa0mi0)

This CL merge two checks into one single check. check_localizability are modified into a utils file, and check_localizable_resources will also run those localizability checks when the files are visited. By doing this, we avoid the extra call to node during presubmit, and all files and nodes will only be visited once.

Also adding these fixes
https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2370103
Fix:
- Pass the fullpath for comparison.(itemPath)
- Normalize comparison between paths
- using native fs.promises instead of promisify (nodejs/node#26581)

Bug: 1116989
Change-Id: I054040d83a65b5f798a21c040096422e287bc799
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2381320
Reviewed-by: Vidal Diazleal <vidorteg@microsoft.com>
Reviewed-by: Tim van der Lippe <tvanderlippe@chromium.org>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Commit-Queue: Christy Chen <chrche@microsoft.com>
babot pushed a commit to binaryage/dirac that referenced this pull request Sep 4, 2020
The current Loc presubmit are doing two checks: check_localizable_resources and check_localizability
(Details of what they are checking: https://docs.google.com/document/d/1L6TkT2-42MMQ72ZSBMFwUaq7M6mDgA2X0x8oHHKaV_U/edit#heading=h.w1no7qaa0mi0)

This CL merge two checks into one single check. check_localizability are modified into a utils file, and check_localizable_resources will also run those localizability checks when the files are visited. By doing this, we avoid the extra call to node during presubmit, and all files and nodes will only be visited once.

Also adding these fixes
https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2370103
Fix:
- Pass the fullpath for comparison.(itemPath)
- Normalize comparison between paths
- using native fs.promises instead of promisify (nodejs/node#26581)

Bug: 1116989
Change-Id: I054040d83a65b5f798a21c040096422e287bc799
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2381320
Reviewed-by: Vidal Diazleal <vidorteg@microsoft.com>
Reviewed-by: Tim van der Lippe <tvanderlippe@chromium.org>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Commit-Queue: Christy Chen <chrche@microsoft.com>
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. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.