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 instance #366

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

lukechilds
Copy link
Contributor

Resolves #365

Made the change mentioned in #365. Also fixed tests to work with new syntax.

@spion
Copy link
Contributor

spion commented Jul 30, 2017

While its a good idea, this is also likely to break a lot of apps using node-sql

@lukechilds
Copy link
Contributor Author

@spion Yep, would definitely have to be SemVer major.

Should just be a search and replace to update though:

const sql = require('sql');

to:

const Sql = require('sql');
const sql = new Sql();

Which is probably what most developers intended to implement.

@lukechilds
Copy link
Contributor Author

lukechilds commented Jul 30, 2017

Or to make upgrading super easy we could wrap the export in a function:

module.exports = new Sql(DEFAULT_DIALECT, {});

would become

module.exports = () => new Sql(DEFAULT_DIALECT, {});

And then you could literally do a search/replace to upgrade:

require('sql') -> require('sql')()

But personally I'd go with the first option.

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.

Don't export Sql instance by default
2 participants