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: add fs/promises alias module #31553

Merged
merged 1 commit into from Feb 19, 2020
Merged

Conversation

@devsnek
Copy link
Member

devsnek commented Jan 28, 2020

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
@devsnek devsnek requested review from ljharb, MylesBorins, cjihrig and nodejs/tsc Jan 28, 2020
@devsnek devsnek added fs and removed build labels Jan 28, 2020
@devsnek devsnek force-pushed the devsnek:restore-fs-slash-promises branch from 1db0897 to aff5f29 Jan 28, 2020
Copy link
Member

mcollina left a comment

LGTM

lib/fs/promises.js Outdated Show resolved Hide resolved
@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 28, 2020

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 devsnek force-pushed the devsnek:restore-fs-slash-promises branch from aff5f29 to 8890701 Jan 28, 2020
@devsnek

This comment has been minimized.

Copy link
Member Author

devsnek commented Jan 28, 2020

@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

This comment has been minimized.

Copy link
Member

ljharb commented Jan 28, 2020

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

This comment has been minimized.

@lpinca
lpinca approved these changes Jan 28, 2020
Copy link
Member

MylesBorins left a comment

LGTM

simple and elegant!

@MylesBorins MylesBorins requested a review from jasnell Jan 29, 2020
@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Jan 29, 2020

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

This comment has been minimized.

Copy link
Member

MylesBorins commented Jan 29, 2020

Should this be semver-minor?

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@devsnek

This comment has been minimized.

Copy link
Member Author

devsnek commented Jan 30, 2020

ping @jasnell

@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Jan 31, 2020
Copy link
Member

Trott left a comment

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

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 31, 2020

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?

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 31, 2020

@devsnek devsnek force-pushed the devsnek:restore-fs-slash-promises branch from 8890701 to 1cdccbe Jan 31, 2020
@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Jan 31, 2020

Going back to the original PR text:

It will fail to load on a version of node that doesn't have it because the fs module on npm is locked.

This is not accurate. require('fs/promises') will not fail on a version of node.js that doesn't have it. Just tested on Node.js 10.x, 11.x, 12.x, and 13.x and it loads ./node_modules/fs/promises.js perfectly well. The fact that a module named fs cannot e published to the npm registry is not relevant.

It is not a new core module, it is a child of the fs module, so the concern about namespacing shouldn't apply

Given the above, it does qualify as a new core module. That alone doesn't block this as there is nothing blocking us at all from adding a new core module. Doing so just means this has to be semver-major.

@jasnell jasnell added semver-major and removed semver-minor labels Jan 31, 2020
doc/api/fs.md Show resolved Hide resolved
@devsnek

This comment has been minimized.

Copy link
Member Author

devsnek commented Jan 31, 2020

@jasnell I realize it can still be loaded, but I'm not sure where people would find such a module because node owns the relevant namespaces.

If it is really needed, I can modify the internal resolver to match names that start with core module names.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Jan 31, 2020

but I'm not sure where people would find such a module because node owns the relevant namespaces.

No one really owns node_modules and there's absolutely nothing in the implementation that treats anything '{core_module}/{whatever}' as being hierarchically tied to '{core_module}'. How someone would find such a module is really not at issue.

Node.js' behavior here is not defined or controlled by what npm allows or denies.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Jan 31, 2020

If it is really needed, I can modify the internal resolver to match names that start with core module names.

This would also force this to be a semver-major change

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 31, 2020

CITGM (queued): https://ci.nodejs.org/job/citgm-smoker/2251/

It appears that the resolve package will break with this change. I'm guessing this line needs to be updated. I'm not sure if the proper things to do is to ignore the CITGM failures for the resolve package and update it after this change makes it into a release or if there is a way to get things patched up ahead of time. /ping @ljharb

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 31, 2020

@Trott until i know what exact released node version this PR will land in, I can't update resolve; so either this has to be merged and released first, or, a firm commitment would have to be made to a version number in advance.

However, if a release proposal which includes this is created, i can update resolve prior to it being released, which would allow cigtm to pass on the release proposal prior to release.

Copy link
Member

tniessen left a comment

This is what I would have preferred from the beginning!

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Feb 3, 2020

This is what I would have preferred from the beginning!

This is actually what it was in the beginning :-) ... It was switched reluctantly to fs.promises due to potential security concerns.

@jasnell jasnell removed the lts-watch-v12.x label Feb 3, 2020
@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Feb 3, 2020

Removed lts-watch given that this is semver-major and can't be backported.

@nodejs-github-bot

This comment has been minimized.

@hiroppy
hiroppy approved these changes Feb 6, 2020
@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Feb 6, 2020

So a thought. If we were to move forward with a nodejs: namespace the we could expose this API within that namespace in a semver-minor way.

we could also in theory expose it as nodejs:promises/fs.

Another possibility, although gross, would be to explicitly check the filesystem on 12.x and use that while printing a VERY VERY VERY clear error if it exists.

Just trying to think of a path forward that would allow something on 12 that supported this feature since using fs.promises in ESM is a terrible DX right now.

@devsnek

This comment has been minimized.

Copy link
Member Author

devsnek commented Feb 6, 2020

adding nodejs: is semver major 'cause filenames can have : in them. my pr that stops node from reading the filesystem for core module scopes is also semver major. i don't see a way to add this without it being semver major unless we put aside the filesystem reads (which i'm happy to do but others are not).

PR-URL: #31553
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
@devsnek devsnek force-pushed the devsnek:restore-fs-slash-promises branch from 1cdccbe to b8e4177 Feb 19, 2020
@devsnek devsnek merged commit b8e4177 into nodejs:master Feb 19, 2020
19 of 20 checks passed
19 of 20 checks passed
build-docs
Details
build-docs
Details
build (3.8)
Details
build (3.8)
Details
build-linux
Details
build-linux
Details
build-windows
Details
build-windows
Details
build-macOS
Details
build-macOS
Details
lint-addon-docs
Details
lint-addon-docs
Details
lint-cpp
Details
lint-cpp
Details
lint-md
Details
lint-md
Details
lint-js
Details
lint-js
Details
Travis CI - Pull Request Build Errored
Details
Travis CI - Branch Build Passed
Details
@devsnek devsnek deleted the devsnek:restore-fs-slash-promises branch Feb 19, 2020
@devsnek

This comment has been minimized.

Copy link
Member Author

devsnek commented Feb 19, 2020

landed in b8e4177

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Feb 19, 2020

since this is semver-major, and it landed, does that make master v14, not v13 anymore?

@devsnek

This comment has been minimized.

Copy link
Member Author

devsnek commented Feb 19, 2020

@ljharb master is already v14.0.0-pre

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Feb 19, 2020

Master is always the next major -pre... That is, currently master is 14.0.0-pre. 13.x is a separate branch.

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.