How promise APIs would look like in core

[balupton]

Proposing this thread to consolidate the various discussions and proposals around "How promise APIs would look like in core." as the original pull request covers more than just that sole issue, making it hard to keep track of what is actually being proposed. Feel free to edit. [CD: Thanks!]

Proposals for current APIs:

Active proposals:

• require('fs').readFilePromise (current PR proposal) + require('fs').promised.readFile shortcut
  https://github.com/nodejs/node/pull/5020#issuecomment-183980246
• require('fs/promise').readFile (future PR proposal) (to immediately follow 5020 once landed)

Current / unresolved discussions:

• do any of the active proposals support callbacks at all? if so, when, why and how? if not, why or when?
• require('fs/promise').readFile initially, or require('fs').readFilePromise initially
  • arguments regarding require('fs/promise').readFile initially and perhaps only
    • +1 works better with ES6 imports https://github.com/nodejs/node/pull/5020#issuecomment-184016883
    • ~~+1 allows blacklisting https://github.com/nodejs/promises/issues/16#issuecomment-184696679~~ [CD: For top level methods, yes, but not for class instance methods. Still best to block promises using global.Promise = null in all cases.]
    • ~~-1 requires PR to be updated https://github.com/nodejs/promises/issues/16#issuecomment-184592603~~ [CD: I am not concerned about changing the PR.]
    • [CD: my concerns with blocking the merge of 5020 on this approach.]
      • -1 unclear on what to do for instance methods (net.Socket#setTimeout)
      • -1 unclear on whether or not to re-export non-promise-returning methods (fs.createReadStream)
      • -1 difficult to drive consensus within collaborator group on whether or not to add submodule
      • -1 difficult to drive consensus on what to call the module (promised is a frontrunner, since we have the name, but this is a potential for bikeshedding)
  • arguments regarding require('fs').readFilePromise with or without require('fs').promised.readFile shortcut initially
    • -1 doesn't work well with ES6 imports https://github.com/nodejs/node/pull/5020#issuecomment-184016883
      • [CD: the promised shortcut works for destructuring assign, import x as y syntax should work for ES2015 modules].
    • -1 despite being in the PR, no consensus has formed yet, also if support is added for all three (not just require('fs/promise') then maintaining 3 endpoints that all do the same thing is pain https://github.com/nodejs/node/pull/5020#issuecomment-179452337
    • +1 already implemented in the PR so can be merged in sooner https://github.com/nodejs/promises/issues/16#issuecomment-184592603
    • +1 solution extends to methods of class instances
  • are there any arguments differentiating between require('fs').readFilePromise and require('fs').promised.readFile, why one or the other?
• should promises wrap callback functions, or should callback functions wrap promise functions (initially or later)

Closed proposals:

• Promise only: require('fs').readFile returns promises only, no support for callback — one API endpoint to maintain, b/c break for everyone, consistent return types, promises are primary and callbacks are eliminated, could provide cleanest forward API
  • argument against here https://github.com/nodejs/promises/issues/16#issuecomment-184726740 b/c break is just too intense
• Callback or promise: require('fs').readFile returns promise if no callback is provided, if callback is provided do not return promise and use callback — one API endpoint to maintain, performance hit with additional if check, different return types, callbacks or promises become secondary to the other
  • argument against here https://github.com/nodejs/node/pull/5020#discussion_r51464838 as it overloads return values
  • argument against here https://github.com/nodejs/node/pull/5020#discussion_r51466441 as it will cause issues with things like process.send(message[, sendHandle][, callback]) which can have a return value despite having an async callback, changing return value to promise will break b/c with current return value
• Promise always with callback support: require('fs').readFile returns promise always, if callback is provided attach it to the promise — one API endpoint to maintain, performance hit for callback users, consistent return types, promises are primary and callbacks become secondary
  • argument against here https://github.com/nodejs/node/pull/5020#discussion_r51466441 as it will cause b/c breaks with things like process.send(message[, sendHandle][, callback]) which can have a return value despite having an async callback, changing return value to promise will break b/c with current return value

Proposals for error APIs:

Discussion ongoing at #10, summary:

[CD - There may not need to be an error API — it might look like a flag, like --abort-on-sync-rejection]

// https://github.com/nodejs/promises/issues/10#issue-133802245
// recovery object
// [CD - It appears likely that we'll go this route since error symposium folks have raised concerns about it.]
await fs.readFilePromise('some/file', {
  ENOENT() {
    return null
  }
})

// normal use:
fs.readFilePromise('some/file')

// https://github.com/nodejs/promises/issues/10#issuecomment-184375707
// [CD - It's unlikely that we'll go this route because I'd like to avoid subclassing Promise.]
let file = await fs.getFileAsync('dne').recover({
   EISDIR(err) {}
   ENOENT(err) { ... }
});

Proposals for new APIs:

[CD - these probably will not land in nodejs/node#5020].

• util.promised: https://github.com/nodejs/promises/issues/5
• util.toCallback: https://github.com/nodejs/promises/issues/6

---

The other issue "How to make sure promises don't get in the way of callback users with core support." seems like a sub-topic of "How promise APIs would look like in core". So perhaps that should be consolidated here too. [CD - Promises will be added using a promisify wrapper, without affecting the original function. They should not interfere with callback APIs at all.]

[chrisdickinson]

Thanks for the issue. I will keep this updated with the current state of the proposal as it evolves.

[balupton]

Cool :-) Some further suggestions:

• [ ] Perhaps the top post of this thread should go to a wiki page, so we have change history tracked. Not sure if necessary, but would be nice. Would also prevent over-writing at the same time.
• [ ] The original post of https://github.com/nodejs/node/pull/5020 should probably point to this repo now, especially as many of the links in the original post no longer take me anywhere due to github thread clipping. E.g. clicking the link in "Why use the promisify approach? Response summed up here." doesn't do anything for me. I'm not sure if the quote I'm going to start cataloguing common questions tonight, and I'll include them under this notice so that you can quickly look to see if there's an answer to your issue without having to search through the entire thread. is referring to something upcoming, or referring to this repo, or referring to this blog post. Update: Added a comment to the PR, original top post still needs update.
• [ ] The medium post https://medium.com/@isntitvacant/adding-promise-support-to-core-a4ea895ccbda mentions to PR as the best place to participate, is that still the case, or will it now be this repo? If the latter, medium post should probably be updated - not a priority as the PR should link to wherever the right place is anyway, just another hop for the user.
• [x] A new thread to discuss error APIs, that this thread can just link to. Not sure if detaching the two is a good idea or not, but seems the medium post goes into a lot of extra detail about error APIs that we may not want to include here. Update: There are now several issues around error handling on this repo, so solved.

[balupton]

It would also be good to have links to the why behind the proposals, like:

• Why require('fs').readFilePromise + require('fs').promised.readFile now then require('fs/promise').readFile later? Why not just one? Why not just require('fs/promise') now?
• Why was this decision made: Promises will be added using a promisify wrapper, without affecting the original function. They should not interfere with callback APIs at all.

The original top post contained some links to discussions, but now it is more a summary now. The medium post goes into plenty more detail, but I can see it becoming out of date once again.

Perhaps consolidating the original post of the PR, the medium post, and the top post here, into one wiki page on this repo, that the WG can keep up to date, and link to the reasonings, then use these individual repo issue threads to facilitate discussions around the particular issues.

[chrisdickinson]

@balupton: Part of the issue with those links into the GitHub issue is that they work, but they take a really long time to load — GitHub appears to load the initial comments quickly, but then pauses on an extra XHR before jumping to the anchor, and that appears to take the bulk of the time.

[benjamingr]

I'm +1 on require("fs/promise"). What are the technical limitations for that?

[chrisdickinson]

@benjamingr: The technical requirements should be pretty straightforward given the way that the promise API is exposed now, but there's significant work to be done in bringing a conversation about such an approach to consensus. I believe this would best be accomplished by a follow-on PR once nodejs/node#5020 is merged (assuming it will be!), to be merged before unflagging, which is the current plan of action.

[balupton]

@chrisdickinson Could doing multiple PRs mean that each merged PR's API is locked in? That seems to be the risk here.

[chrisdickinson]

@balupton I do not think so — we are working behind a flag, and the next step after the initial PR is merged is to build up a "go / no go" checklist for unflagging. We can (if we so choose) delay unflagging on getting a satisfactory answer to the require('fs/promise') discussion. The initial PR gets us to the point that we can work behind a flag.

[benjamingr]

+1 for require("fs/promise") vs require("fs").promised blocking unflagging but nor PR.

+1 for error object/.recover not blocking the PR.

[phpnode]

Bikeshed: I firmly believe it's safer and more sensible to call this require('fs/2') instead of require('fs/promise'). This gives us a safe path to introducing changes which would otherwise cause compatibility problems in future. For instance if whatwg streams were accepted in future, they could go into require('stream/2'). This gives node the ability to evolve its API over time without throwing existing code under the bus.

[chrisdickinson]

@phpnode: Noted, but this can be discussed on the subsequent PR. It's not likely that a /2 moniker will be uncontentious, since that naming scheme implies that callbacks are an older, and thus invalid, pattern — which is not the case.

[phpnode]

I agree that it might be contentious but the reason is social not technical, whereas the benefits of using /2 is technical - it lets us evolve the API. If some unknown new language feature comes along in the next few years (maybe Observables) and we want to adopt that in node, what is our path, especially when there is fs and fs/promise? This is obvious when modules are explicitly versioned - fs/3.

The counter point is that explicitly requiring fs/promise makes promises feel second class, and it is non-obvious to newcomers why they'd need to include that when they're thinking in terms of async and await, whereas saying "use the latest version of the fs module" is more straightforward.

I don't think it implies that callbacks are invalid, they're not being deprecated, but they are older.

[benjamingr]

@phpnode I think that this attitude where callbacks are "older" and promises are "the next API which people should default to using" will not ever get promises into node core.

Callbacks were there first so suggesting "fs/callbacks" and "fs/promises" is not an option. I'm personally completely fine with "fs/promise"

[phpnode]

I think that this attitude where callbacks are "older" and promises are "the next API which people should default to using" will not ever get promises into node core.

Agree, I'm not suggesting that, and definitely not suggesting fs/callbacks. The main reason I have a slight problem with fs/promise is the future upgrades issue, I don't personally look forward to a world where we have fs, fs/promise and fs/observable for instance. So part of this discussion should probably focus on how those future similar types of changes would be accommodated.

[benjamingr]

@phpnode observables would not replace promises but would rather work in areas Node works with streams and event emitters. No one is suggesting replacing all callbacks with observables.

Observables and async iterators are the problems of the streams working group - not us :)

[phpnode]

@benjamingr I'm not talking about observables specifically, rather how this kind of disruptive change is accommodated in future - in this case we're talking about promises, maybe next time we'll be talking about something equally disruptive. Observables is just an example.

[balupton]

If compat ever breaks, which is too early to tell, it could always become require('fs/promises/2') — fragmenting entire APIs under a single versioning scheme this early seems weird. What happens if the callback APIs change, would that be require('fs/3') which also includes promises but doesn't as require('fs/2') was for promises and not callbacks… doesn't really make sense.

[balupton]

Tangent comment moved to https://github.com/nodejs/promises/issues/10#issuecomment-184703584

[petkaantonov]

A completely separate module can be blacklisted so that concerns such as promises ruining existing post-mortem workflows with callbacks can be addressed by simply blacklisting the separate module as people with such concerns already have to do for 3rd party modules.

[omsmith]

+1 for require("fs/promise") vs require("fs").promised blocking unflagging but nor PR.

I'll echo that.

Don't think I'd want to see .promised end up as an actual API, but if it's felt it's a necessary stepping stone at this point - go for it.

[balupton]

Regarding the surface promise API, the only other suggestion that is not covered is just updating the existing API methods to use promises instead or with callbacks, so no new methods are introduced, just existing methods are updated. This has probably been discussed somewhere, if so would be good to have the conclusion summarised in the first/original post.

Update: updated the initial post with the various proposals and their counter arguments

[benjamingr]

Regarding the surface promise API, the only other suggestion that is not covered is just updating the existing API methods to use promises instead or with callbacks, so no new methods are introduced

This was discussed already in the pull request. Namely https://github.com/nodejs/node/pull/5020#discussion_r51464838

We're also not going to break "fs" or any of the existing modules.

[omsmith]

@chrisdickinson Apologies if I missed it in the PR thread (was hard to track all that). Would you (or someone else) be able to sum up:

• Is the reasoning of wanting to land with .promised and follow-up with a fs/promise is just to see something land?
• Is it believed .promised will be more likely to get landed than a separate module?

[balupton]

@benjamingr cheers, I've updated the initial post with the counters so future people know they are ruled out and why

[balupton]

@omsmith

Is the reasoning of wanting to land with .promised and follow-up with a fs/promise is just to see something land?

Seems so. I've updated the top post with the relevant links.

Is it believed .promised will be more likely to get landed than a separate module?

I'm too confused about any difference in benefits between require('fs').readFilePromise and require('fs').promised.readFile - haven't spotted anything on it. More details on this would be nice.

[omsmith]

Benefits of having fs/promise and/or the .promised shortcut:

• nit: I don't have Async or Promise all over
• nit: The presented functions are only Promise returning ones, which is what I actually wanted

Benfits of having only fs/promise:

• Callback consumers do not have to load promisified functions into memory

[balupton]

Hrmm...

If fs/promise also supports callbacks (probably by attaching the callback to the promise if it exists), then one just ever needs to include it for both callback and promise support, never needing to include both require('fs') and require('fs/promised') if for some reason the codebase is large and they need both

nit: The presented functions are only Promise returning ones, which is what I actually wanted

This is a nifty point as another side of this coin is that considering the process.send(message[, sendHandle][, callback]) return type issue as raised in https://github.com/nodejs/node/pull/5020#discussion_r51466441 - a promise version of process.send will break return types, so require('process/promise').send which is now promise based will need different documentation and considerations concerning that b/c break, so require('process').sendPromised could provide a sense of false of compatibility

[pesho]

Benefits of having only fs/promise:

Also:

• Ability to blacklist promisified APIs altogether (mentioned by @petkaantonov above)

[balupton]

Callback consumers do not have to load promisified functions into memory

Seems a strange argument as one only needs at least two modules in their entire app where one module uses the promise API and another module uses the callback API, for this to benefit to be negated, which as time goes by will become highly likely. Unless one of the modules (probably the promise API) is specifically blacklisted somehow, which would cause module restrictions which is perhaps undesirable — is such a blacklisting ever likely to occur?

[omsmith]

Perhaps, but I think it follows along with some comments I saw in the original PR:

• Don't want to affect the callback functions, performance wise (increased base memory usage could be seen as an effect)
• Node.js is used in many different environments, including embedded systems (we should try to respect the restrictions of low-memory environments)
• A lot of work goes into reducing resource usage and start-up times
• A lot of work goes into making sure core maintains a small surface area (while fs/promise would still increase of surface area of core, it at least doesn't increase the surface area of fs)

My own thought:

• Bundlers such as Browserify provide shims for core modules like fs. Adding the Promise returning methods inline will bloat those bundles as well (although minimally, thanks to the way it's been approached by chrisdickinson)