Skip to content

Don't export instance - #366

Open
lukechilds wants to merge 6 commits into
brianc:masterfrom
lukechilds:dont-export-instance
Open

Don't export instance#366
lukechilds wants to merge 6 commits into
brianc:masterfrom
lukechilds:dont-export-instance

Conversation

@lukechilds

Copy link
Copy Markdown
Contributor

Resolves#365

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

@spion

Copy link
Copy Markdown
Contributor

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

@lukechilds

Copy link
Copy Markdown
ContributorAuthor

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

Should just be a search and replace to update though:

constsql=require('sql');

to:

constSql=require('sql');constsql=newSql();

Which is probably what most developers intended to implement.

@lukechilds

lukechilds commented Jul 30, 2017

Copy link
Copy Markdown
ContributorAuthor

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

module.exports=newSql(DEFAULT_DIALECT,{});

would become

module.exports=()=>newSql(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 freeto 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

@lukechilds@spion