Skip to content
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
Merged

Allow using custom Client/Dialect #1428

merged 4 commits into from
May 20, 2016

Conversation

vellotis
Copy link
Contributor

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
Copy link
Contributor Author

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) {
Copy link
Member

@rhys-vdw rhys-vdw May 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@vellotis vellotis May 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, you're right.

@rhys-vdw
Copy link
Member

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 knex:master May 20, 2016
@vellotis
Copy link
Contributor Author

BR 🎉

@rhys-vdw
Copy link
Member

👍

@elhigu
Copy link
Member

elhigu commented May 22, 2016

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

@igorklopov
Copy link

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants