New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow using custom Client/Dialect #1428

Merged
merged 4 commits into from May 20, 2016

Conversation

Projects
None yet
4 participants
@vellotis
Contributor

vellotis commented May 19, 2016

Solve #1424

To oppose @rhys-vdw note in #1424.

I stand to my suggested solution. Client API is really much knex version dependent. If some version of knex would change the API, the plugin-client would stop working any way. So there is no point supporting any other soft verification.

Two tests added.

  • One for currently present feature
  • One for the new feature

As well PR #1427 merging with this PR would be welcomed.

vellotis added some commits May 19, 2016

@vellotis

This comment has been minimized.

Contributor

vellotis commented May 19, 2016

Hmm... test fails for some unknown reason. All ok on my own branch https://travis-ci.org/vellotis/knex/builds/131377631. The same commit 41344a9.

@@ -25,6 +25,8 @@ export default function Knex(config) {
let Dialect;
if (arguments.length === 0 || (!config.client && !config.dialect)) {
Dialect = makeClient(Client)
} else if (typeof config.client === 'function' && config.client.prototype instanceof Client) {

This comment has been minimized.

@rhys-vdw

rhys-vdw May 20, 2016

Collaborator

This can be simplified to:

} else if (config.client instanceof Client) {

instanceof will walk the prototype list. It will return false if config.client is not a function.

This comment has been minimized.

@vellotis

vellotis May 20, 2016

Contributor

@rhys-vdw I must say that you are not right. You are not passing initialized Client but the definition of the Client. Lets see it in node cli.

> Client = require('knex').Client
{ [Function: Client] ... }

> MysqlClient = require('knex/lib/dialects/mysql')
{ [Function: Client_MySQL] ... }

> MysqlClient instanceof Client
false

> MysqlClient.prototype instanceof Client
true

This comment has been minimized.

@rhys-vdw

rhys-vdw May 20, 2016

Collaborator

Sorry, you're right.

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented May 20, 2016

Hmm... test fails for some unknown reason.

Don't sweat it. Sometimes the tests time out. I can just re-run that individual test (you can see most node versions passed).

If you push a commit addressing my line note the tests will re-run anyway.

@rhys-vdw rhys-vdw merged commit 25ca625 into tgriesser:master May 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vellotis

This comment has been minimized.

Contributor

vellotis commented May 20, 2016

BR 🎉

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented May 20, 2016

👍

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 22, 2016

I think this would really need documentations since it is new feature... I'll open issue for it.

@igorklopov

This comment has been minimized.

igorklopov commented Jul 18, 2016

FYI i made use of custom dialect feature. The only problem i see is a wierd way of getting ancestors:
https://github.com/igorklopov/firebird-knex/blob/master/src/transaction.js#L5
Can you export Transaction, ColumnCompiler, etc from knex same way as Knex.Client and Knex.Promise:
https://github.com/tgriesser/knex/blob/master/src/index.js#L41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment