Thanks for the great package! There's one thing I noticed that threw me off and could potentially cause some nasty bugs.
By default require('sql') exports an instance of Sql which then gets cached by require, meaning any future require('sql') statements will return the same instance of Sql.
I think this is bad because it means if a project is using two separate packages that depend on sql they will most likely be using the same Sql instance which I don't think most developers would expect. This could be especially nasty if you were using two packages with separate dialects.
As a very simple example:
mysql.js
const mysql = require('sql');
mysql.setDialect('mysql');
module.exports = mysql;
sqlite.js
const sqlite = require('sql');
sqlite.setDialect('sqlite');
module.exports = sqlite;
app.js
const mysql = require('./mysql.js');
const sqlite = require('./sqlite.js');
mysql.dialect;
// sqlite
sqlite.dialect;
// sqlite
I don't think this is the behaviour most developers would expect. It can obviously be solved easily by the developer explicitly using the Sql constructor:
const Sql = require('sql').Sql;
const mysql = new Sql();
mysql.setDialect('mysql');
module.exports = mysql;
However I don't think this is obvious. I would suggest exporting Sql as the default export as a breaking change and publishing a new major version.
Migrating on the user end should be trivial. It would just be a matter of developers changing:
const sql = require('sql');
to:
const Sql = require('sql');
const sql = new Sql();
Which was probably what they were intending to do anyway.
Thanks for the great package! There's one thing I noticed that threw me off and could potentially cause some nasty bugs.
By default
require('sql')exports an instance ofSqlwhich then gets cached byrequire, meaning any futurerequire('sql')statements will return the same instance ofSql.I think this is bad because it means if a project is using two separate packages that depend on
sqlthey will most likely be using the sameSqlinstance which I don't think most developers would expect. This could be especially nasty if you were using two packages with separate dialects.As a very simple example:
mysql.jssqlite.jsapp.jsI don't think this is the behaviour most developers would expect. It can obviously be solved easily by the developer explicitly using the
Sqlconstructor:However I don't think this is obvious. I would suggest exporting
Sqlas the default export as a breaking change and publishing a new major version.Migrating on the user end should be trivial. It would just be a matter of developers changing:
to:
Which was probably what they were intending to do anyway.