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
Merged

Update tarn.js #3345

merged 3 commits into from Jul 11, 2019

Conversation

kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Jul 10, 2019

No description provided.

@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad 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
Copy link
Member

@elhigu elhigu 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$/
Copy link
Collaborator Author

@kibertoad kibertoad Jul 11, 2019

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

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;
Copy link
Contributor

@alexandrebodin alexandrebodin Jul 11, 2019

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

@elhigu elhigu Jul 11, 2019

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

@alexandrebodin alexandrebodin Jul 11, 2019

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

@kibertoad kibertoad Jul 11, 2019

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

@elhigu elhigu Jul 11, 2019

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

@kibertoad kibertoad Jul 11, 2019

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.

@kibertoad kibertoad merged commit fb06464 into master Jul 11, 2019
3 checks passed
@kibertoad kibertoad deleted the chore/update-tarn branch Jul 11, 2019
felixmosh pushed a commit to felixmosh/knex that referenced this issue Jul 13, 2019
* Update tarn.js
* Remove beforeDestroy
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.

None yet

3 participants