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

Don't export Sql instance by default #365

Open
lukechilds opened this issue Jul 30, 2017 · 0 comments · May be fixed by #366
Open

Don't export Sql instance by default #365

lukechilds opened this issue Jul 30, 2017 · 0 comments · May be fixed by #366

Comments

@lukechilds
Copy link
Contributor

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.

@lukechilds lukechilds linked a pull request Jul 30, 2017 that will close this issue
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 a pull request may close this issue.

1 participant