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

Update tarn.js #3345

Merged
merged 3 commits into from Jul 11, 2019

Conversation

@kibertoad
Copy link
Collaborator

commented Jul 10, 2019

No description provided.

@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2019

@elhigu @koskimas .afterCreate() hook explodes as unsupported now. We don't support and don't want to support that anymore? Listening to creation events is the way to go now?

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

afterCreate is implemented in knex side and looks like it is passed to pool paameters by accident.

@@ -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$/

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 11, 2019

Author Collaborator

Original assertion was failing on Windows

@elhigu
elhigu approved these changes Jul 11, 2019
};
// afterCreate is an internal knex param, tarn.js does not support it
if (tarnPoolConfig.afterCreate) {
delete tarnPoolConfig.afterCreate;

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Jul 11, 2019

You will have the same issue with beforeDestroy I think.

This comment has been minimized.

Copy link
@elhigu

elhigu Jul 11, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Jul 11, 2019

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

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 11, 2019

Author Collaborator

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.

This comment has been minimized.

Copy link
@elhigu

elhigu Jul 11, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 11, 2019

Author Collaborator

Yeah, migration guide already mentions pool event handlers.

@kibertoad kibertoad merged commit fb06464 into master Jul 11, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 88.229%
Details

@kibertoad kibertoad deleted the chore/update-tarn branch Jul 11, 2019

felixmosh added a commit to felixmosh/knex that referenced this pull request Jul 13, 2019
Update tarn.js (tgriesser#3345)
* Update tarn.js
* Remove beforeDestroy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.