This repository was archived by the owner on Mar 31, 2018. It is now read-only.
Repository navigation
This repository was archived by the owner on Mar 31, 2018. It is now read-only.
How promise APIs would look like in core #16
Description
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.readFileshortcut
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').readFileinitially, orrequire('fs').readFilePromiseinitially- arguments regarding
require('fs/promise').readFileinitially 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 usingglobal.Promise = nullin 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 (
promisedis a frontrunner, since we have the name, but this is a potential for bikeshedding)
- -1 unclear on what to do for instance methods (
- arguments regarding
require('fs').readFilePromisewith or withoutrequire('fs').promised.readFileshortcut initially- -1 doesn't work well with ES6 imports Adding Core support for Promises node#5020 (comment)
- [CD: the
promisedshortcut works for destructuring assign,import x as ysyntax should work for ES2015 modules].
- [CD: the
- -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
- -1 doesn't work well with ES6 imports Adding Core support for Promises node#5020 (comment)
- are there any arguments differentiating between
require('fs').readFilePromiseandrequire('fs').promised.readFile, why one or the other?
- arguments regarding
- should promises wrap callback functions, or should callback functions wrap promise functions (initially or later)
Closed proposals:
- Promise only:
require('fs').readFilereturns 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').readFilereturns 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').readFilereturns 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
- argument against here Adding Core support for Promises node#5020 (comment) as it will cause b/c breaks with things like
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 #5util.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.]
Activity
chrisdickinson commentedon Feb 16, 2016
Thanks for the issue. I will keep this updated with the current state of the proposal as it evolves.
balupton commentedon Feb 16, 2016
Cool :-) Some further suggestions:
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.balupton commentedon Feb 16, 2016
It would also be good to have links to the why behind the proposals, like:
require('fs').readFilePromise+require('fs').promised.readFilenow thenrequire('fs/promise').readFilelater? Why not just one? Why not justrequire('fs/promise')now?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 commentedon Feb 16, 2016
@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 commentedon Feb 16, 2016
I'm +1 on
require("fs/promise"). What are the technical limitations for that?chrisdickinson commentedon Feb 16, 2016
@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 commentedon Feb 16, 2016
@chrisdickinson Could doing multiple PRs mean that each merged PR's API is locked in? That seems to be the risk here.
chrisdickinson commentedon Feb 16, 2016
@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 commentedon Feb 16, 2016
+1 for
require("fs/promise")vsrequire("fs").promisedblocking unflagging but nor PR.+1 for error object/
.recovernot blocking the PR.phpnode commentedon Feb 16, 2016
Bikeshed: I firmly believe it's safer and more sensible to call this
require('fs/2')instead ofrequire('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 intorequire('stream/2'). This gives node the ability to evolve its API over time without throwing existing code under the bus.chrisdickinson commentedon Feb 16, 2016
@phpnode: Noted, but this can be discussed on the subsequent PR. It's not likely that a
/2moniker will be uncontentious, since that naming scheme implies that callbacks are an older, and thus invalid, pattern — which is not the case.phpnode commentedon Feb 16, 2016
I agree that it might be contentious but the reason is social not technical, whereas the benefits of using
/2is 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 isfsandfs/promise? This is obvious when modules are explicitly versioned -fs/3.The counter point is that explicitly requiring
fs/promisemakes promises feel second class, and it is non-obvious to newcomers why they'd need to include that when they're thinking in terms ofasyncandawait, 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 commentedon Feb 16, 2016
@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"21 remaining items
mikeal commentedon Feb 16, 2016
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 commentedon Feb 16, 2016
@mikeal I've emailed the owner to see if they'd be willing to transfer the package.
omsmith commentedon Feb 16, 2016
Just to note, that would only be necessary for
require('promised/foo'), as opposed torequire('foo/promised')which I think has generally been the discussion point.chrisdickinson commentedon Feb 16, 2016
@omsmith Indeed, but given some thought I believe nesting under
promisedis going to be (technically) a bit more tenable than nesting under each module.benjamingr commentedon Feb 17, 2016
Update: The author of "promised" has transferred ownership to the Node foundation (for @chrisdickinson )
chrisdickinson commentedon Feb 17, 2016
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:
--enable-promisesflag, Introduce*Promisemethods for most single-occurrence async operations in the Node API, excluding streams and stream-pair generating methods. This is Adding Core support for Promises node#5020.require('promise/*')set of modules behind the flag.petkaantonov commentedon Feb 17, 2016
@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 commentedon Feb 17, 2016
@petkaantonov Yep — I saw — the problem is that since there are promisified methods on class instances, disallowing access to promise methods via
requirewon't be enough to blacklist them.omsmith commentedon Feb 17, 2016
@chrisdickinson which methods on which class instances?
EDIT: just noticed the notes you put inline on the top comment. I see you mean
net.Socketmethods etc.petkaantonov commentedon Feb 17, 2016
what about "promised/net" that returns "net" while adding promise methods on the first require?
omsmith commentedon Feb 17, 2016
.promised?createReadStreamseperatelychrisdickinson commentedon Feb 18, 2016
@petkaantonov As I understand it, that would mean either we'd have to copy/re-run the
netcode to get our own copy ofSocket, or that we'd mutate the non-promise-variant ofSocket, which would be visible to other users.petkaantonov commentedon Feb 18, 2016
yes, thats what promisifyAll does. Having those methods only when a blacklistable module is required rather than all the time is surely better?
chrisdickinson commentedon Feb 18, 2016
@petkaantonov To clarify, you're suggesting we mutate
require('net').Socket.prototypewhen a user runsrequire('promised/net')? Or that we re-run thenetcode?petkaantonov commentedon Feb 18, 2016
Oh sorry I thought you were familiar with the api. It mutates the prototype (non destructively).