CSNW / sql-bricks

Transparent, Schemaless SQL Generation
http://csnw.github.io/sql-bricks
MIT License
204 stars 25 forks source link

Dialect Extension mechanism #43

Closed prust closed 9 years ago

prust commented 9 years ago

This pull request fixes #36.

Suor commented 9 years ago

I need to say that I really not happy with new require API. It's both verbose and unobvious. People will try to var sql = require('sql-bricks') cause that's the standard way of doing things in node and will be confused. Also, it's far too much code to start it using for any dialect. And people don't use sql standarts they use real dbs.

What do you think of:

var sql = require('sql-bricks').configure() // default
var sql = require('sql-bricks').configure({dialect: 'postgres'}) // or
var sql = require('sql-bricks').configure(
    {extend: [require('./custom-layer'), 'postgres']})

Additional params like placeholder type could be also passed to .configure().

Another thing to keep in mind is not to think to much about custom layers, cause almost nobody would be writing them. The usual case people want (me including) is just - give a generator for my db. So, I'll go with a dialiect param and leave extending for future.

prust commented 9 years ago

@Suor: You're right. I was concerned that allowing a simple require('sql-bricks') (or SqlBricks in the browser) because that is a single, global namespace, but as long as require('sql-bricks').configure(...) returns a new namespace, rather than modifying the global namespace, that should take care of that problem. How about this:

var sql = require('sql-bricks'); // default
var sql = require('sql-bricks').configure({dialect: 'postgres'}); // returns new, non-conflicting namespace

Yeah, I'm not sure about {extend: ...}, let's leave that until we need it. I agree about people not using standards or wanting to mix in standards (though they would want to mix in features...)

prust commented 9 years ago

@Suor: Thank you for your suggestions & constructive criticism. I took a pass at implementing this.

(Also, I exposed inherits() on the main sql-bricks namespace. I checked and the base Statement class was already exposed.)

For starters, I like reverting back to a simple require('sql-bricks') -- I agree that require('sql-bricks')() was unintuitive and a bad idea.

But after I implemented .configure({dialect: ...}), I realized that this made the core SQLBricks library depend on the various dialect libraries instead of vice-versa. This makes it harder for people to fork and extend -- they would need to fork the dialect library and fork the core library and make it point to their forked dialect. This also makes it harder for there to be competing dialect libraries (if the first one becomes unmaintained or buggy, I want it to be easy for people to create a competing one).

So instead, I dropped .configure() and spun out separate dialect libraries that depend on the sql-bricks repo: https://github.com/CSNW/sql-bricks-postgres and https://github.com/CSNW/sql-bricks-sqlite.

So instead of:

var sql = require('sql-bricks').configure({dialect: 'postgres'});

It's just:

var sql = require('sql-bricks-postgres');

You're welcome to fork https://github.com/CSNW/sql-bricks-postgres and add to it. Or, if you'd prefer, you can just fork sql-bricks itself and add your dialect-specific extensions to it directly. Either way, once you have a public repository that has the functionality of https://github.com/CSNW/sql-bricks-postgres, I'll probably delete my sql-bricks-postgres and encourage people to use yours instead. I can also give you control of http://npmjs.org/package/sql-bricks-postgres (or you're welcome to publish yours under a different name, like "sql-bricks-pg", if you want).