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

@vellotis 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 4 commits May 19, 2016
@vellotis
Copy link
Contributor Author

@vellotis 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
Member

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
Author 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
Member

Sorry, you're right.

@rhys-vdw
Copy link
Member

@rhys-vdw 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 knex:master May 20, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vellotis
Copy link
Contributor Author

@vellotis vellotis commented May 20, 2016

BR 🎉

@rhys-vdw
Copy link
Member

@rhys-vdw rhys-vdw commented May 20, 2016

👍

@elhigu
Copy link
Member

@elhigu 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 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants