lib: re-export promises for esm

[devsnek]

fs.promises, dns.promises, (and soon readline.promises) should all be exposed nicely to esm. This does so.

cc @nodejs/modules @addaleax @cjihrig

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

[targos]

I believe we originally exposed the fs promises API this way. I don't remember why we reverted to fs.promises.

[devsnek]

@targos there were (and still are) unresolved issues about namespacing, but since it seems that's literally never going to be resolved, i thought i would move this forward.

[mcollina]

@devsnek would you mind adding some recap of those issues and how those were fixed?

[devsnek]

@mcollina the issue was nodejs: vs @nodejs/ and no one ever came to an agreement afaik.

[mcollina]

@devsnek I don't think that was it. I think we reverted that because of some issues around module lookup and security. Essentially if somebody creates a node_modules/fs/promises folder on an old version of Node.js, they will be able to load that file.

@jasnell should know more.

[jasnell]

The reason we had to switch to fs.promises was security and consistency! On any version of Node.js prior to the addition of fs/promises, require('fs/promises') would fallback to searching the file system, which meant that any third-party module could actually install it's own implementation of what fs/promises without the user being aware that they were getting something different than what they expected.

[devsnek]

@jasnell that's true of all new modules we add though

[Trott]

If I understand correctly (and I very much might not):

• Namespacing: I don't think the hold-up was bikeshedding on the name as much as lack of consensus on whether we should even do it. There are people who feel like the friction of not having a namespace--the way it makes us careful about adding new modules--is a feature and not a bug. I disagree with that analysis but I understand it certainly. There are also people who believe the namespacing ship sailed a long time ago and it's too late now because we'll never get rid of existing non-namespaced modules.

• Using / vs. . was an unexpected-filesystem-override thing as noted by @jasnell. Your point that it applies to all new modules we add is well-taken but it's also one reason (of several) why we don't add module names that conflict with existing modules that have more than trivial usage.

All of this is, IMO, a mess that would be resolved by simply namespacing modules going forward. If it's a big problem for ESM (which I have not been following closely), that's just all the more reason for the TSC to take this up and make it happen. I would certainly support that.

[devsnek]

@Trott i would definitely appreciate if the TSC took this up

currently in esm:

import { promises as fsPromises } from 'fs';
const { readFile } = fsPromises;

after you write this a few times it really starts to grate on you.

[ljharb]

this constitutes a new core module - and there's not supposed to be any new ones until #21551 (comment) is resolved. that's also why fs/promises was reverted.

[MylesBorins]

@ljharb that is not why fs/promises was reverted. @jasnell can chime in but if I recall there were security concerns

[BridgeAR]

@jasnell

The reason we had to switch to fs.promises was security and consistency! On any version of Node.js prior to the addition of fs/promises, require('fs/promises') would fallback to searching the file system, which meant that any third-party module could actually install it's own implementation of what fs/promises without the user being aware that they were getting something different than what they expected.

That is also possible right now. Just add an own fs to the require cache. That way all following fs calls resolve to the manually added one instead of the native one. We even have a test to guarantee that this behavior is not broken: test/parallel/test-require-cache.js.

[BridgeAR]

I personally think it's still not ideal that we went for a new object property instead of choosing to add Promise or similar in the function name and export it with all the other functions. That way we'd not always have struggles like these. It would also allow to immediately identify the function as working differently from the callback based versions by name. It also simplifies the handling for e.g. fs since we only export the specific promise based functions on the fs.promises property and all static properties are on the main property. So users have to use both parts in case they want to use promises.

[MylesBorins]

@devsnek how would you feel about making a nodejs:promises export?

import {fs} from 'nodejs:promises || import {readFile} from 'nodejs:promises/fs

Thoughts?

[devsnek]

@MylesBorins anything that allows import { readFile } from is fine by my standard. nodjes:promises/fs is a bit heavy in terms of length and non-alpha characters, but it seems usable.

[MylesBorins]

@devsnek v cool. Let's see if we can reach agreement in the namespace issue... if we can I'd like to include the promises export in that PR