Skip to content
This repository has been archived by the owner on Mar 31, 2018. It is now read-only.

Add pattern examples #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 119 additions & 0 deletions patterns/conversion.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
# Working with callbacks

This page explains how promises work with APIs that return callbacks.

When users have a callback API in a format like:

###1. One time event:

```js
process.on("rejectionHandled", function(e) { // I only ever want to deal with this once

});
```

###2. Plain non-standard callback:

```js
function request(onChangeHandler){
...
request(function(){
// change happened
});
```

###3. Standard style callback ("nodeback"):

```js
function getStuff(dat,callback){
...
getStuff("dataParam",function(err,data){

}
```
###4. A whole library with node style callbacks:

```js
API;
API.one(function(err,data){
API.two(function(err,data2){
API.three(function(err,data3){

})
});
});
```

###How do I work with the API in promises, how do I "promisify" it?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need a space between # and H here.


First of all, there are plans to add support to promises in core. You can track
that here: https://github.com/nodejs/node/pull/5020

APIs should only be converted to return promises if they do not already return promises.

Promises have state, they start as pending and can settle to:

- __fulfilled__ meaning that the computation completed successfully.
- __rejected__ meaning that the computation failed.

Promise returning functions _should never throw_, they should return rejections instead.
Throwing from a promise returning function will force you to use both a `} catch { ` _and_ a `.catch`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example here would be very helpful.

People using promisified APIs do not expect promises to throw. If you're not sure how async
APIs work in JS - please [see this answer](http://stackoverflow.com/questions/14220321) first.

###1. DOM load or other one time event:

So, creating promises generally means specifying when they settle - that means when they move
to the fulfilled or rejected phase to indicate the data is available (and can be accessed with `.then`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might reword this — may I suggest:

Creating a promise directly usually means choosing the point at which it should settle.

Once we land the glossary we can link settle to the appropriate glossary term.


```js
function load() { // our event
return new Promise(function(resolve,reject) { // create a new promise
process.on("rejectionHandled", resolve); // resolve it when the event happens
});
}
```

###2. Non-Standard callback:

These APIs are rather common since well... callbacks are common in JS. In Node it is strictly prefered

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preferred

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest omitting strictly, since Node itself breaks from this convention at times (fs.exists).

that callback code you write uses the standard `(err, data)` convention.

Let's look at the common case of having `onSuccess` and `onFail`:

function getUserData(userId, onLoad, onFail){ ...

This would look like the following

```js
function getUserDataAsync(userId) {
return new Promise(function(resolve,reject) {
getUserData(userId, resolve, reject); // pass reject/resolve handlers explicitly
});
}
```

###3. Standard style callback ("nodeback"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should define nodeback in the glossary and prefer that term's use when describing the err-first callback pattern.


Node style callbacks (nodebacks) have a particular format where the callbacks is always the last
argument and its first parameter is an error. Let's first promisify one manually:

getStuff("dataParam",function(err,data){

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code highlight is missing


To:

```js
function getStuffAsync(param) { // a function that takes the parameter without the callback
return new Promise(function(resolve,reject) { // we invoke the promise constructor
getStuff(param,function(err,data) { // call the function internally
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this try/catch the fn as well, since it's common for functions taking a nodeback to throw if input params are badly shaped?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new Promise already catches all errors and rejects the promise.

if(err) return reject(err); // if it errord we reject
resolve(data); // otherwise re resolve

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we

});
});
}
```
Notes:

- Of course, when you are in a `.then` handler you do not need to promisify things.
Returning a promise from a `.then` handler will resolve or reject with that promise's value.
Throwing from a `.then` handler is also good practice and will reject the promise.
84 changes: 84 additions & 0 deletions patterns/disposer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
# The Promise Disposer Pattern

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really a pattern? Can you provide some sources? (articles, examples of code, etc)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, yes. It's pretty common, I've given two examples of popular libraries using it - I can find more if you'd like. There is also general stuff like http://promise-nuggets.github.io/articles/21-context-managers-transactions.html and more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern accomplishes same things as python's with, C#'s using, Java's try-with-resources, C++'s RAII. Here's a example http://promise-nuggets.github.io/articles/21-context-managers-transactions.html

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you've linked to issues that were opened by you, so it might look like something that you are promoting

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh thanks, I copied most of this content here from StackOverflow. I opened those two (and indeed both libraries use the pattern now) but I can gladly link to other libraries I had nothing to do with that happen to use the pattern. It's pretty common and the reason I linked to it from the StackOverflow post is that it includes the debate which I thought was interesting.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjamingr, @petkaantonov I would not compare it with RAII though, its too different. The others are specifically designed for managing non-memory resources in a garbage collected language where RAII isn't available anymore.

@vkurchatkin its true, we introduced this feature in bluebird and I haven't seen other libraries adopt it. I guess most people consider it trivial as its simplest implementation can be easily written with .finally().

I'd say its effective difference from finally is non-trivial though, because returning disposers lets API authors ensure that a resource cannot be used in any other way other than by passing it in a using closure. And doing that guarantees that regardless of whether the block was successful, or any exceptions or errors happened, it will call the resource's disposer when done. Finally it also provides the one important place where node really does need to crash: if the code that releases (scarce) resources throws or asynchronously fails.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, this is not an established pattern and it probably shouldn't be here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not established in JavaScript in the sense that I haven't seen another library provide it out of the box. Bluebird users are using it.

I would say its an established pattern in other languages, since they've added special official keywords for it (C# using keyword, Python with keyword, Java try-with-resources). And AFAIK, they are all based on the finally part of try-catch-finally in the same way that this pattern is.

So to be honest, while its technically not "established", its obviousness and equivalency to finally-based patterns I think earns its keep. If thats not enough, we could remove it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say its an established pattern in other languages

we are talking specifically about promise-based patterns. finally is already a part of javascript

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finally is not analogous to any of those synchronous patterns (those languages even contain separate finally statement), especially when promises are concerned since it's very easy to cause leaks when using finally to cleanup parallel promises (this is a strong motivator for the using method in bluebird in the first place)


Often we have to perform resource management in code. For example - release a DB connection:


```js
var users = getDb().then(conn =>
conn.query("SELECT name FROM users").then(users => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return is missing here
Edit: didn't see it was an arrow function, sorry!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll convert it to a multi-line lambda for clarify - that's how the node docs behave so I might as well align - thanks.

Copy link
Member

@juliangruber juliangruber Aug 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this be cleaner written using promise caching? i can see a shitstorm break loose about "this is callback hell again", something like

NVM this doesn't let you catch the error event.

var users = getDb().then(conn => conn.query("SELECT name FROM users"))
users().then(() => conn.release()).catch(e => { conn.release(), throw e })
return users()

conn.release();
return users;
}, e => {
conn.release();
throw e;
})
});
```

The problem with the above approach is that if you forget releasing the connection after every single
time you perform `getDb` you have a resource leak that might freeze your app eventually
when it runs out of the resource you're leaking.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add that its not just about forgetting, its about easily writing code that stays resilient in the face of potential exceptions, which is an important concern when promises are used. You could have conn.release() in your code (so you haven't forgotten it). Yet if you perform operations between acquiring the connection and calling conn.release(), they could throw. Because we don't crash and we have more exceptions flying around when using promises, its far more likely that anything can unsuspectingly interrupt the execution flow at any time. This pattern significantly reduces cognitive burden in those cases - resources used with it cannot be leaked.

Its basically the pattern that addresses the shank-shiz concern for promises.


The above approach means that the user has to manually manage resources.

The user might in one place do:

```js
var users = getDb().then(function(conn) {
return conn.query("SELECT name FROM users");
});
```

Which will leak a database connection that was never closed.

The disposer pattern is a way to couple a scope of code with owning the resource. By binding the resource
to a scope we make sure it is always released when we're done with it and we can't easily forget
to release it. It is similar to `using` in C#, `with` in Python and try-with-resource in Java
as well as RAII in C++.


It looks like:

```js
withResource(function(resource) {
return fnThatDoesWorkWithResource(resource); // returns a promise
}).then(function(result) {
// resource disposed here
});
```

If we wrote our code as:

```JS
function withDb(work) {
var _db;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks cleaner with var db; and db = _db

return myDbDriver.getConnection().then(function(db) {
_db = db; // keep reference
return work(db); // perform work on db
}).then(v => {
if (_db) {
_db.release();
}
return v;
}, e => {
if(_db) {
_db.release();
}
throw e;
});
}
```

We could write our above code as:

```js
withDb(conn =>
conn.query("SELECT name FROM users");
).then(users => {
// connection released here
});
```

Userland examples of users of the disposer pattern are [sequelize](https://github.com/sequelize/sequelize/issues/1939)
and [knex](https://github.com/tgriesser/knex/issues/265) (bookshelf's query builder). It's also possible to use it
for simpler things like hiding a loader when all AJAX requests completed for instance.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight typo in this line — all AJAX requests completed — maybe insert have between requests and completed?

46 changes: 46 additions & 0 deletions patterns/explicit-construction.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
##The Explicit Construction Anti-Pattern

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some hash or double? It is not consistent with the other file. Also, there should be a space after the hash for readability right?


This is the most common anti-pattern. It is easy to fall into this when you haven't really grasped
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This is a common practice anti-pattern" — it would be hard to back up the assertion that it's the "most common", since we're relying mostly on (lots of) anecdotal data here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually did collect real data at a point, but for example the question about it in StackOverflow is referenced by over 300 questions in stack overflow http://stackoverflow.com/questions/linked/23803743?lq=1 being the single most linked question about promises in SO.

promises. It's also sometimes called the promise constructor anti-pattern. Let's recap: promises
are about making asynchronous code retain most of the lost properties of synchronous code such
as flat indentation and one exception channel.

This pattern is also called the deferred anti-pattern.

In the explicit construction anti-pattern, promise objects are created for no reason, complicating code.

First example is creating a new promise object when you already have a promise or thenable:

```js
function getFullName(userId) {
return new Promise(function(resolve, reject) {
db.getUsers(userId).then(data => {
resolve(data.fullName);
});
});
}
```

This superfluous wrapping is also dangerous, any kind of errors and rejections are swallowed and
not propagated to the caller of this function.

Instead of using the above the code should simply return the promise it already has and propagate

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using the above code

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm. I think there has to be a comma

values using `return`:

```js
function getFullName(userId) {
return db.getUsers(userId).then(data =>
data.fullName)
);
});
}
```

Not only is the code shorter but more importantly, if there is any error it will propagate properly to
the final consumer.

**So when should the promise constructor be used?**

Well simply, when you have to.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we nix this, or change it to read "Rarely, usually only for wrapping a callback API."?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe adding "and even in this case it's safer to use a library that does this while covering all the edge cases"


You might have to use explicit wrapping object when wrapping a callback API.