fs: add fs/promises alias module#31553

devsnek · 2020-01-28T16:34:07Z

This restores fs/promises in node core. It can't be overridden by a file on disk in a version of node that has it. It will fail to load on a version of node that doesn't have it because the fs module on npm is locked. It is not a new core module, it is a child of the fs module, so the concern about namespacing shouldn't apply.

Fixes#21014

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

mcollina

LGTM

lib/fs/promises.js

ljharb · 2020-01-28T17:08:43Z

The concern applies because fs had no “children” before, and things like resolve identify core modules by the full name, not just the first segment. I expect CIGTM to fail with this change.

devsnek · 2020-01-28T17:11:15Z

@ljharb it seems resolve already has an entry for it, and the v8/tools entries appear to support disjoint version matching, so I think resolve should be able to be fixed pretty easily.

The namespacing concern is from adding new things that occupy space in global registries such as npm, not about breaking modules that have a list of all the things you can successfully require in node.

ljharb · 2020-01-28T17:13:06Z

It does, due to 10.0.0, that is true, and yes, i can easily add the next version - but it will be marked as a non-core module until users update to the latest version of resolve.

nodejs-github-bot · 2020-01-28T18:27:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28741/

MylesBorins

LGTM

simple and elegant!

MylesBorins · 2020-01-29T05:01:10Z

I'd like to see @jasnell sign-off on this as I believe he was one of the folks who identified earlier security concerns. To be clear I'm extremely happy to see this land!

MylesBorins · 2020-01-29T05:01:28Z

Should this be semver-minor?

nodejs-github-bot · 2020-01-29T05:10:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28750/

nodejs-github-bot · 2020-01-29T18:26:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28764/

devsnek · 2020-01-30T23:16:01Z

ping @jasnell

nodejs-github-bot · 2020-01-30T23:16:47Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28790/

Trott

LGTM but I'm inclined to run CITGM in case of unforeseen consequences?

Trott · 2020-01-31T00:25:58Z

Should there be a test that does something like this?:

assert.strictEqual(require('fs/promises'),require('fs').promises)

Otherwise, I don't think we're testing CommonJS require('fs/promises') anywhere?

Refs: nodejs#31553 Refs: nodejs#32953

Refs: #31553 Refs: #32953 PR-URL: #34001 Refs: #34002 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refs: nodejs#31553 Refs: nodejs#32953 Refs: nodejs#33950 Refs: nodejs#34001 Refs: nodejs#34002

Refs: nodejs#31553 Refs: nodejs#32953 Refs: nodejs#33950 Refs: nodejs#34001 Refs: nodejs#34002 Refs: nodejs#34055

Refs: #31553 Refs: #32953 Refs: #33950 Refs: #34001 Refs: #34002 Refs: #34055 PR-URL: #34962 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>

Refs: #31553 Refs: #32953 Refs: #33950 Refs: #34001 Refs: #34002 PR-URL: #34055 Refs: #34962 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Fixes: nodejs#35740 Refs: nodejs#31553 Refs: nodejs#32953 Refs: nodejs#33991 Refs: nodejs#34001 Refs: nodejs#34055 Refs: nodejs#34962 Co-authored-by: Antoine du Hamel <[email protected]> Co-authored-by: Trivikram Kamat <[email protected]> Co-authored-by: Rich Trott <[email protected]>

Fixes: #35740 Refs: #31553 Refs: #32953 Refs: #33991 Refs: #34001 Refs: #34055 Refs: #34962 Co-authored-by: Antoine du Hamel <[email protected]> Co-authored-by: Trivikram Kamat <[email protected]> Co-authored-by: Rich Trott <[email protected]> PR-URL: #34002 Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Masashi Hirano <[email protected]> Reviewed-By: Pranshu Srivastava <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

Refs: #31553 Refs: #32953 Refs: #33950 Refs: #34001 Refs: #34002 Refs: #34055 PR-URL: #34962 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>

Refs: #31553 Refs: #32953 Refs: #33950 Refs: #34001 Refs: #34002 PR-URL: #34055 Refs: #34962 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Fixes: #35740 Refs: #31553 Refs: #32953 Refs: #33991 Refs: #34001 Refs: #34055 Refs: #34962 Co-authored-by: Antoine du Hamel <[email protected]> Co-authored-by: Trivikram Kamat <[email protected]> Co-authored-by: Rich Trott <[email protected]> PR-URL: #34002 Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Masashi Hirano <[email protected]> Reviewed-By: Pranshu Srivastava <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

devsnek requested review from a team, MylesBorins, cjihrig and ljharb January 28, 2020 16:34

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 28, 2020

devsnek added fs Issues and PRs related to the fs subsystem / file system. and removed build Issues and PRs related to build files or the CI. labels Jan 28, 2020

devsnek force-pushed the restore-fs-slash-promises branch from 1db0897 to aff5f29Compare January 28, 2020 16:41

mcollina approved these changes Jan 28, 2020
View reviewed changes

targos reviewed Jan 28, 2020
View reviewed changes

lib/fs/promises.js Outdated Show resolvedHide resolved

devsnek force-pushed the restore-fs-slash-promises branch from aff5f29 to 8890701Compare January 28, 2020 17:08

devsnek mentioned this pull request Jan 28, 2020
Can’t import fs Promises API with ECMAScript Modules #21014
Closed

lpinca approved these changes Jan 28, 2020
View reviewed changes

cjihrig approved these changes Jan 28, 2020
View reviewed changes

MylesBorins approved these changes Jan 29, 2020
View reviewed changes

MylesBorins requested a review from jasnell January 29, 2020 05:00

MylesBorins added the lts-watch-v12.x label Jan 29, 2020

devsnek added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 29, 2020

Trott approved these changes Jan 31, 2020
View reviewed changes

ExE-Boss added a commit to ExE-Boss/node that referenced this pull request Jun 21, 2020
assert: add assert/strict alias module
e1e6de7
Refs: nodejs#31553 Refs: nodejs#32953

ExE-Boss mentioned this pull request Jun 21, 2020
doc: document changes for */promises alias modules #34002
Closed
2 tasks

ExE-Boss mentioned this pull request Jun 25, 2020
util: add util/types alias module #34055
Closed
4 tasks

ExE-Boss added a commit to ExE-Boss/node that referenced this pull request Jun 27, 2020
util: add util/types alias module
86bc1db
Refs: nodejs#31553 Refs: nodejs#32953 Refs: nodejs#33950 Refs: nodejs#34001 Refs: nodejs#34002

addaleax mentioned this pull request Jun 27, 2020
timers: introduce timers/promises #33950
Closed
4 tasks

richardlau mentioned this pull request Jul 16, 2020
doc: remove reference to fs/promises #34393
Closed
2 tasks

ExE-Boss mentioned this pull request Aug 28, 2020
path: add path/posix and path/win32 alias modules #34962
Closed
4 tasks

ExE-Boss added a commit to ExE-Boss/node that referenced this pull request Oct 6, 2020
path: add path/posix and path/win32 alias modules
4c75a27
Refs: nodejs#31553 Refs: nodejs#32953 Refs: nodejs#33950 Refs: nodejs#34001 Refs: nodejs#34002 Refs: nodejs#34055

Uh oh!

fs: add fs/promises alias module#31553

fs: add fs/promises alias module #31553

Uh oh!

Conversation

devsnek commented Jan 28, 2020

Checklist

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ljharb commented Jan 28, 2020

Uh oh!

devsnek commented Jan 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Jan 28, 2020

Uh oh!

nodejs-github-bot commented Jan 28, 2020

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Jan 29, 2020

Uh oh!

MylesBorins commented Jan 29, 2020

Uh oh!

nodejs-github-bot commented Jan 29, 2020

Uh oh!

nodejs-github-bot commented Jan 29, 2020

Uh oh!

devsnek commented Jan 30, 2020

Uh oh!

nodejs-github-bot commented Jan 30, 2020

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Jan 31, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

devsnek commented Jan 28, 2020•
edited
Loading

Trott commented Jan 31, 2020•
edited
Loading