balderdashy / waterline-docs

WARNING: The content in this repo is out of date! See https://github.com/balderdashy/sails-docs for the most up-to-date documentation
452 stars 163 forks source link

Removed the promisey pyramid of doom #95

Closed BLamy closed 9 years ago

BLamy commented 9 years ago

There seems to be a few things wrong with this code snippet.

1) The catch block should be on the outside promise. If User.create fails you will have no idea. 2) When passing console.log or console.error into then or catch you should bind this to console

3) Is it really necessary to save the relationship going backwards as well? Granted I'm not a waterline expert but I don't remember having to do this when working with sails.

I understand that you guys nested the promise because you needed to capture user. But if you don't need to save it back it would probably be best not having an example with a nested promise. Lots of people don't fully understand promises and seeing nested promises as examples will just lead to them creating promise callback hell.

devinivy commented 9 years ago

I think this is good :+1:

Is there a less confusing behavior we can use than console.log.bind(console)? Seems a little distracting for the docs.

BLamy commented 9 years ago

It's a little bit confusing however, it it technically the right way to do it, the docs should teach good habits. People will be very surprised when they copy paste code and it doesn't work as intended.

Perhaps we can add some comments like this


    User.create({ // First we create a user.
            firstName: 'Neil',
            lastName: 'Armstrong'
        }).then(function (user) { // Then we create the pet
            return Pet.create({
                breed: 'beagle',
                type: 'dog',
                name: 'Astro',
                owner: user.id
            });

        }).then(function (pet) { // Then we grab all users and their pets
            return User.find()
                .populate('pets');

        }).then(function(users){ // Results of the previous then clause are passed to the next
             console.dir(users);

        }).catch(console.error.bind(console)); // You can pass a function directly into a then/catch. 
                                              // Use bind to make sure the function is called on the right object.
devinivy commented 9 years ago

I'd almost opt for,

/* ... */
}).catch(function(err) {
    console.error(err);
});

I think one upside is that it is clear how many arguments there are, for those becoming familiarized with promises– and avoiding bind in an example where it's not necessary. Either way this is an improvement to the docs :) . @eddieajau thoughts?

dmarcelino commented 9 years ago

:+1:

devinivy commented 9 years ago

Thanks!