CodeFoodPixels / node-promise-mysql

A wrapper for mysqljs/mysql that wraps function calls with Bluebird promises.
MIT License
338 stars 63 forks source link

Restore ignorant of connection state API #3

Closed gajus closed 9 years ago

gajus commented 9 years ago

This has changed as a result of https://github.com/lukeb-uk/node-promise-mysql/issues/1.

What was the reason that did not allow to have createConnection() return a promise that resolves to a connection state and in addition allows access to query, etc?

Without having had a look at your code, this would look something along the lines of:

var createConnection;

/**
 * @return {Promise} state
 * @return {Function} state.query
 */
createConnection = function (config) {
    var mysql = require('mysql'),
        state;

    state = new Promise(function (resolve) {
        mysql
            .createConnection(config)
            .connect(resolve);
    });

    state.query = function (query, params) {
        return state
            .then(function () {
                // Proceed to 
            });
    };

    return state;
};

Then API allows to do both:

var pm = require('promise-mysql'),
    db;

db = pm.createConnection();

db.then(function () {
    // Connection successful.
});

db.catch(function () {
    // Connection unsuccessful.
});

db.query(function () {
    // Query that is executed after successful connection.
});

I have temporarily monkey-patched this with:

var db = mysql.createConnection(config);

db.query = function (sql, values) {
    return db.then(function (connection) {
        return connection.query(sql, values);
    });
};
CodeFoodPixels commented 9 years ago

I'd rather not modify the promise object though. This method would modify the promise object which could have unknown side effects.

gajus commented 9 years ago

It modifies an instance of Promise that has a definitive use case. If you don't feel confident with that, you can isolate the promise altogether, e.g.

state = new Promise(/* .. */);

return {
    then: state.then,
    catch: state.catch,
    query: /* .. */
};
CodeFoodPixels commented 9 years ago

Except the promise exposes a lot more than then and catch. There's no reason that I should remove all this functionality from the promise. See https://github.com/petkaantonov/bluebird/blob/master/API.md for more details of the bluebird promise.

gajus commented 9 years ago

I am very well aware of that. But you are talking about 0.01% use cases (in this specific scenario). There is nothing holding that minority to re-extend their Promise object wrapping it again in Bluebird, which is possible, as long as you provide these methods: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Promise.

CodeFoodPixels commented 9 years ago

As I have said in this thread, I don't want to modify the promise object nor do I want to restrict what people can use on it.

If you feel differently, feel free to submit PR or even take a fork of the code (It's MIT licensed).