Ensure that 'client' is provided in knex config object

[wubzz]

The option is documented as required, but it's currently not being validated. This leads to strange side effects such as ones describe in #1628.

I also took the liberty of adding modifiers to the base columpcompiler as it currently does not have this property, but it should.

[elhigu]

looks good

[eddieajau]

There is a small regression with this. Previously when calling knex migrate:make the config option was not required. Now knex migrate:make errors because Knex is not correctly configured in the local development environment (but would be configured correctly when knex was actually run in a production environment).

[venables]

I disagree with throwing the exception here. One of the documented features of Knex is:

You can even use knex without a connection, just for its query building features. Just pass in an empty object when initializing the library. Specify a client if you are interested in a particular flavour of SQL.
var knex = require('knex')({});
knex('table').insert({a: 'b'}).returning('*').toString();
// "insert into "table" ("a") values ('b')"

I think we should revert the Exception bit, and treat the bug as fixed by the PR by adding this.modifiers = [].

[elhigu]

Looks like this one actually was a breaking change, since there kind of was documented. I'm fine with reverting and releasing fixed version as 0.12.8 with explication in changelog.

I don't like the feature of initializing builder without dialect, but it clearly was intended to work. @wubzz does it sound ok to you? or was there actually documentation somewhere that client was required configuration parameter?

[elhigu]

Actually yeah... other issue actually did state the place where it was mentioned.

Initializing the Library

The knex module is itself a function which takes a configuration object for Knex, accepting a few parameters. The client parameter is required and determines which client adapter will be used with the library.

So I think we should remove the invalid part of the documentation. I would prefer to have client as required, because currently trying to use knex without client is very badly tested and probably will keep on breaking in the future.

But since there was inconsistent documentation about the feature this should have been made with minor version bump instead of patch version...

[venables]

Thanks @elhigu - I have found the "client-less" initializer to have been helpful over the years, but I understand your concerns.

I am happy to help write better test coverage for the feature if that's desired.

Another option would be to allow setting the client like client: "none" -- forcing it to be explicitly called out could help prevent user errors and confusion (which seems to be the goal here, right?)

[elhigu]

@venables I still don't understand why clientless initialization is more helpful that for example just using { client: 'pg' } initialization? I understand initializing without connection to be able to test queries, but without client the generated SQL is really not runnable anywhere...

Anyways if that feature is really wanted and better tested in that case { client : "none" } sounds like a good way to initialize the clientless builder.

[venables]

I won't have all the use-cases for clientless initialization, but the use-cases that I've hit professionally have been:

1. Unit tests. We use this feature heavily in our tests. The knex project uses/used it in tests as well.
2. Needing to define a Bookshelf model before the database connection is established. We Initialized Bookshelf with a client-less Knex instance and swapped the Knex instance once Knex was connected.

All things we can work around, but I think it is a really great feature -- even if it were hid behind some sort of advanced none setting which uses the base schema class.

This is a feature that Knex was advertising as a good thing, and I agree.

[wubzz]

@elhigu I still stand firm that 'client' should always be required. It's unfortunate that the documentation has double standards. The use cases above don't sound too convincing to me. One could just as easily supply client in unit tests and the second use case sounds like a 'cheap trick' (no offense) that the library should support in a different way.

[elhigu]

@venables In both of those use cases one can initialize { client: 'pg' } or { client: 'sqlite3' }. Only thing which just will be inconvenient is that unit test result checking might need to be fixed a little. However { client: 'pg' } query generation is very near to current clientless output.

I still have not seen any case where { client: 'none' } would be any better choice that { client: 'pg' } or any other real client without giving connection details for query unit testing.

You mentioned about using the base schema class... indeed most of the dialect differences are in schema. Is there any specific reasons why using base schema would be desired instead of some real schema of real dialect?

For now I feel that the best choice would be to revert throwing the error, update changelogs and release 0.12.8.

Then reapply the fix and release 0.13.0 and mark it as a breaking change and fix the documentation.

[venables]

@wubzz @elhigu Thanks for the response. The unit test use-case was my primary concern, but it looks like the database connection pool is not actually created until a query is executed -- which might be another piece of confusion from the documentation:

Initializing the library should normally only ever happen once in your application, as it creates a connection pool for the current database, you should use the instance returned from the initialize call throughout your library.

So I think we should be all set by setting our tests to client: 'pg' like you suggest, since the database connection would not be established upon initialization.

Thank you again for looking into this.

I'll close the Revert PR -- I'll leave the decision to you guys about 13.0 vs 12.8

[TimonLukas]

My use case for this was to use Knex with IBMs DB2. Without specifying a client it actually generated valid queries, which the other client types do not seem to do. Oh well, I just resorted to manually writing my queries.