Dirty extensions

[Suor]

The way extensions are implemented they extend core namespace and classes inplace. So if anyone uses shared sql-bricks.js, like in browser or npm dedupe environment, things could clash. See how sql is mutated here:

nesh> var sql = require('./node_modules/sql-bricks');
undefined
nesh> sql.values
undefined
nesh> var pgsql = require('./postgres');
undefined
nesh> pgsql.values
{ [Function: Values]
  super_: [Function: Statement],
  defineClause: [Function] }
nesh> sql.values
{ [Function: Values]
  super_: [Function: Statement],
  defineClause: [Function] }

[prust]

@Suor: right -- this is why I originally proposed an additional function call, the () at the end of require('sql-bricks')(). But I think you're right: that API was unintuitive.

With the way node caches modules, we can't make require('sql-bricks') return something different inside sql-bricks-postgres than it does in the end-user's program. The only alternative I can think of is for sql-bricks-postgres and other extensions to use a different API than normal users, something like require('sql-bricks').extend(...), which could return a new, separate, namespace that could be extended...

Does this sound like a good approach? Can you think of other options?

[Suor]

There is no connection of sql-bricks-postgres and friends require interface and dirtying. We just need to stop writing to shared sql namespace and its constructors.

An extension can export prototype descendant instead:

var pgsql = Object.create(sql);
module.exports = pgsql;

And maybe inherit constructors with sql.inherits:

function Select() {
    if (!(this instanceof Select))
      this = Object.create(Select.prototype);

    Select.super_.apply(this, arguments);
    return this;
}
pgsql.select = sql.inherits(Select, sql.Select);

[prust]

@Suor:

var pgsql = Object.create(sql);

I think I see. I don't have much experience with Object.create(), but my understanding is that this will set pgsql's prototype to the sql namespace -- basically using prototype chaining to automatically delegate calls down, but without requiring the user to "instantiate" anything (the user wouldn't need to do var pgsql = new require('sql-bricks-postgres')()). Cool idea, I hadn't thought of that.

I like this direction -- are there any changes I would need to make to the underlying sql-bricks library, or is this just something that would be implemented in extensions (sql-bricks-postgres and sql-bricks-sqlite)?

[Suor]

I'll try updating sql-bricks-postgres this way. Most probably this could be done with no changes to sql-bricks itself. However, some things could be shared to make creating extensions easier. I'll get back when I have something to show.

[Suor]

I managed to undirty my extension - https://github.com/Suor/sql-bricks-postgres/commit/bf6eff88277de1fc30c796dba2bf2cbce2ef54c8. It required some hassle to make subclasses, which have safe constructors and pass all their arguments to base.

Object.create() didn't work cause it's not really mixable with your class-like approach. Things could've been smooth if you used prototypal inheritance in sql-bricks too.

Regarding moving anything to sql-bricks, it's your call. However, makeSubclass() could be moved (optionally stripped from defineClause specific). Or you can move in the whole thing, so that extension would just write:

var pgsql = sql._extension();

pgsql.select.prototype.smth = ...
...

This, however, will clone everything even if it's not needed.

[Suor]

I also needed to copy everything that's not on prototype, but namespace itself - https://github.com/Suor/sql-bricks-postgres/commit/ad302520ed40c69bf48ce2a07974cea5aa0d791b

[prust]

I read through both commits and they look perfect to me; good work @Suor!

Or you can move in the whole thing

This makes the most sense to me -- do you want to submit this as a pull request?

[Suor]

Ok. Tomorrow.

[prust]

For the record, this was addressed in #55, #56 and finally closed in #57.