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

Jump to bottom

Update tarn.js #3345

Merged
merged 3 commits into from Jul 11, 2019
Merged

Update tarn.js #3345

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e7f6ef
Update tarn.js
kibertoad Jul 10, 2019
d7ac481
Do not pass unsupported params to tarn.js
kibertoad Jul 11, 2019
593f9f9
Remove beforeDestroy
kibertoad Jul 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions UPGRADING.md
View file
Open in desktop
@@ -1,5 +1,9 @@
## Upgrading to new knex.js versions

### Upgrading to version 0.19.0+

* Passing unknown properties to connection pool configuration now throws errors (see https://github.com/Vincit/tarn.js/issues/19 for details);

### Upgrading to version 0.18.0+

* Node.js older than 8 is no longer supported, make sure to update your environment;
Expand Down
10 changes: 9 additions & 1 deletion lib/client.js
View file
Open in desktop
Expand Up @@ -297,7 +297,15 @@ Object.assign(Client.prototype, {
return;
}

this.pool = new Pool(this.getPoolSettings(config.pool));
const tarnPoolConfig = {
...this.getPoolSettings(config.pool),
};
// afterCreate is an internal knex param, tarn.js does not support it
if (tarnPoolConfig.afterCreate) {
delete tarnPoolConfig.afterCreate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will have the same issue with beforeDestroy I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beforeDestroy is deprecated and is not really useful hook anyways (it has been deprecated already for a long time). I think we can drop it already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure the PR should simply delete it from the code so there is no way it is passed by error then :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed beforeDestroy from typings and implementation, and mentioned in migration guide that it's no longer supported. Users should still update their code not to pass it, though, as silently ignoring configuration options is confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When people start complaining that their hacky code (*) doesn't work without it we can tell them to use pool's event handlers instead.

*) code relaying to that hook cannot work in a robust manner unless it is counting more or less how many connections were discarded from pool... so far all the people who has been using it and with whom I've been talking to have made invalid assumptions when that is called and have tried to do some DB disconnected kind recognitions and knex re-inits there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, migration guide already mentions pool event handlers.

}

this.pool = new Pool(tarnPoolConfig);
},

validateConnection(connection) {
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
@@ -1,6 +1,6 @@
{
"name": "knex",
"version": "0.18.4",
"version": "0.19.0",
"description": "A batteries-included SQL query & schema builder for Postgres, MySQL and SQLite3 and the Browser",
"main": "knex.js",
"types": "types/index.d.ts",
Expand Down Expand Up @@ -38,7 +38,7 @@
"lodash": "^4.17.14",
"mkdirp": "^0.5.1",
"pg-connection-string": "2.0.0",
"tarn": "^1.1.5",
"tarn": "^2.0.0",
"tildify": "2.0.0",
"uuid": "^3.3.2",
"v8flags": "^3.1.3"
Expand Down
2 changes: 1 addition & 1 deletion test/unit/seed/seeder.js
View file
Open in desktop
Expand Up @@ -90,7 +90,7 @@ describe('Seeder._waterfallBatch', function() {
it('should throw an error with correct file name', (done) => {
seeder._waterfallBatch(['1-first.js', '2-second.js']).catch((error) => {
expect(error.message).to.match(
/^Error while executing "(\/\w+)+\/1-first\.js" seed: throwing in first file$/
/^Error while executing .*1-first.js" seed: throwing in first file$/
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original assertion was failing on Windows

);
done();
});
Expand Down