Repository navigation
Hooks: proper nesting with savepoints, customisation, fix tests - #206
Merged
Merged
Conversation
Member
|
Awesome, thank you for getting this fixed! Released as v5.1.0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instead of simply incrementing a counter
starthook creates a nested transaction with a savepoint. Counter way fails with complex scenarios like new third test for hooks.Added
getKnexoption forstarthook. Allows to specify where to get knex from, so the hook can be used not only withfeathers-knexservice. Useful forfeathers-objectionand tests.Fixed previous test and added two additional. Previous test was actually not working, it was passing simply because "people2" table was not existent. And initial design of replacing "COMMIT;" with "COMMITA;" was a mistake because a connection was not released afterwards (it was working initially because it was the last test, but stopped to be in #181).
Remark on history: as i understand in an attempt to fix the problem caused by the broken test #181 included a removal of some lines in
hooks.js(a, b, c, d), that caused regression in unhandled rejections, so #196 was needed. It had advantage over initial solution in handling rejections from an establishing phase, but ate rejections from an execution phase (this should have been visible if the broken test was working and not turned off in #181). Now both establishing and execution phases should be handled right.