knex / knex

A query builder for PostgreSQL, MySQL, CockroachDB, SQL Server, SQLite3 and Oracle, designed to be flexible, portable, and fun to use.
https://knexjs.org/
MIT License
19.37k stars 2.13k forks source link

`this.dialect` is useful #3384

Closed mytototo closed 5 years ago

mytototo commented 5 years ago

Hi,

I know you want to deprecate this.dialect so we can use config.client instead. Here is the warning:

Using 'this.dialect' to identify the client is deprecated and support for it will be removed in the future. Please use configuration option 'client' instead.

Looks logical.

However, new Dialect() is still useful when we want to use node modules that don't allow dynamic requiring, such as pkg.

Dynamic require is at https://github.com/tgriesser/knex/blob/master/lib/knex.js#L43

Instead of passing a client string, I'd like to keep doing this:

const Dialect = require('knex/src/dialects/postgres/index');
const db = new Dialect({});

This way, I can continue using third-party module that don't support dynamic requiring.

What do you think ? Thank you.

elhigu commented 5 years ago

dialect and client configuration attributes are aliases for the same thing. So deprecating dialect will not affect to anything.

Support for third party dialects will work as described in here: https://github.com/tgriesser/knex/blob/master/CONTRIBUTING.md#i-would-like-to-add-support-for-new-dialect-to-knex-is-it-possible

mytototo commented 5 years ago

Got it, I didn't know this way of using existing dialect. Thank you!