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
  Adding Core support for Promises node#5020 (comment)
• 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 Adding Core support for Promises node#5020 (comment)
    • +1 allows blacklisting How promise APIs would look like in core #16 (comment) [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 How promise APIs would look like in core #16 (comment) [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 Adding Core support for Promises node#5020 (comment)
      • [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 Adding Core support for Promises node#5020 (comment)
    • +1 already implemented in the PR so can be merged in sooner How promise APIs would look like in core #16 (comment)
    • +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 How promise APIs would look like in core #16 (comment) 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 Adding Core support for Promises node#5020 (comment) as it overloads return values
  • argument against here Adding Core support for Promises node#5020 (comment) 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 Adding Core support for Promises node#5020 (comment) 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: util.promised - Callbacks and async/await  #5
• util.toCallback: util.toCallback(asyncFn) - interop of async functions with callbacks. #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 Adding Core support for Promises node#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.
• 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.