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
  nodejs/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 nodejs/node#5020 (comment)
    • +1 allows blacklisting #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 #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 nodejs/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 nodejs/node#5020 (comment)
    • +1 already implemented in the PR so can be merged in sooner #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 #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 nodejs/node#5020 (comment) as it overloads return values
  • argument against here nodejs/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 nodejs/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: #5
• util.toCallback: #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 nodejs/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.

[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 #10 (comment)

[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 nodejs/node#5020 (comment)

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 nodejs/node#5020 (comment) - 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)

[vdeturckheim]

Could it be possible to choose an alternative promise library ? In mongoose, it is possible to pass a constructor that will be used by the package.

Default would be to standard promises, but the possibility to do something like methodReturningPromise.as(MyPromiseConstructor).then... ?

Or better (with all alternatives in discussion):

• require('fs').promised([MyPromiseConstructor])
• require('fs/promise').as(MyPromiseConstructor).readFile where the as method would be optional

[benjamingr]

@vdeturckheim that was discussed and decided against (I think). Please see the original PR's comments.

[kriskowal]

I am in favor of having separate require("promised/*") modules so that the promise oriented porcelain does not get tangled with the underlying modules. The same pattern may one day be called upon to provide a compatibility layer for web streams, e.g., require("web/fs"), which might be both promise oriented and web streams oriented. Regardless, we need to avoid entangling these concerns with the underlying callback and event-emitter oriented design.

[vdeturckheim]

@benjamingr sorry my bad then.

[mikeal]

Keep in mind that using a top level namespace will break anyone using a module named that in the ecosystem, in this case https://www.npmjs.com/package/promised which doesn't seem to be in heavy use. We'd want to talk with the publisher about pulling it and npm about blocking it from being accessible in the future if we go this route.

[chrisdickinson]

@mikeal I've emailed the owner to see if they'd be willing to transfer the package.

[omsmith]

Just to note, that would only be necessary for require('promised/foo'), as opposed to require('foo/promised') which I think has generally been the discussion point.

[chrisdickinson]

@omsmith Indeed, but given some thought I believe nesting under promised is going to be (technically) a bit more tenable than nesting under each module.

[benjamingr]

Update: The author of "promised" has transferred ownership to the Node foundation (for @chrisdickinson )

[chrisdickinson]

I've updated the +1/-1's for the module approach to reflect the concerns with the approach — in short, it will take a lot of intense discussion, and the concrete version of it will spur a lot of debate. Out of the interest of making this process sustainable, the plan of record is to work on getting 5020 merged without a solution for modules, then work to get modules in behind the flag before unflagging in a separate PR. To do otherwise risks the work in nodejs/node#5020 and that we've done here overall, and I'm not comfortable doing that.

While I appreciate the summary of potential plans and their merits/demerits, I'd like to propose we make it reflect the following plan of action:

• Behind the --enable-promises flag, Introduce *Promise methods for most single-occurrence async operations in the Node API, excluding streams and stream-pair generating methods. This is nodejs/node#5020.
  • Once we have established that there are no blocking concerns for merging behind a flag, seek buy-in from the Node collaborator group on merging.
• Once the PR is merged, I will immediately open a PR introducing a require('promise/*') set of modules behind the flag.
  • We will initially consider this PR "blocking unflagging".
• Once we've brought all issues marked "blocking unflagging" to a conclusion, then we will lift the flag. Official support of the API will begin one version later.

[petkaantonov]

@chrisdickinson I am not sure if you saw it in some other thread, but I suggested separate module because such a module can be blacklisted, just like 3rd party promise modules are currently blacklisted by post-mortem users (I assume).

[chrisdickinson]

@petkaantonov Yep — I saw — the problem is that since there are promisified methods on class instances, disallowing access to promise methods via require won't be enough to blacklist them.

[omsmith]

@chrisdickinson which methods on which class instances?

EDIT: just noticed the notes you put inline on the top comment. I see you mean net.Socket methods etc.

[petkaantonov]

what about "promised/net" that returns "net" while adding promise methods on the first require?

[omsmith]

-1 unclear on whether or not to re-export non-promise-returning methods (fs.createReadStream)

• What was the plan to export onto .promised?
• If we look to mz/fs a popular community effort, the non-promise-returning methods are excluded
  • May not be "bluebird-popular", but was the most popular "promisified fs" package I could find, outside of q-io, which is a little different
  • Falls in line with what I would expect
    • Would admittedly be a minor annoyance to have to import createReadStream seperately

[chrisdickinson]

@petkaantonov As I understand it, that would mean either we'd have to copy/re-run the net code to get our own copy of Socket, or that we'd mutate the non-promise-variant of Socket, which would be visible to other users.

[petkaantonov]

yes, thats what promisifyAll does. Having those methods only when a blacklistable module is required rather than all the time is surely better?

[chrisdickinson]

@petkaantonov To clarify, you're suggesting we mutate require('net').Socket.prototype when a user runs require('promised/net')? Or that we re-run the net code?

[petkaantonov]

Oh sorry I thought you were familiar with the api. It mutates the prototype (non destructively).