fix: support async afterEach hook #1242
Conversation
Currently a asynchronous afterEach hook will execute concurrently with the underlying express request lifecycle. This fix allows async hooks to complete before allowing the express request to continue through its middleware stack. Fixes pact-foundation#1241
lhokktyn
force-pushed
the
fix-async-aftereach
branch
from
891566a
to
891e0cb
Compare
src/dsl/verifier/proxy/hooks.ts
Outdated
| @@ -31,19 +31,17 @@ export const registerAfterHook = ( | |||
| config: ProxyOptions, | |||
| stateSetupPath: string | |||
| ): void => { | |||
| logger.trace("registered 'afterEach' hook"); | |||
There was a problem hiding this comment.
This is now slightly misleading- it always registers the hook, but the hook is only connected to something if config.afterEach is set.
There was a problem hiding this comment.
Now checks for presence of configured hooks before logging.
I've got another PR being lined up to refactor this bit a little more, but looking like it'll best for a separate discussion 🙂
lhokktyn
force-pushed
the
fix-async-aftereach
branch
from
891e0cb
to
d368c80
Compare
|
@mefellows just a heads up that I'm lining up another PR that builds on top of this PR, which contains some further refactoring around the hooks stuff - mainly with the aim of ensuring hooks are only run once during each interaction's setup/teardown phase respectively. I've got it marked as a breaking change at the moment - hence wanting to raise as a separate PR - but I'm not sure it is actually a break to be honest so a little discussion around that would be great once this PR is merged if you don't mind. |
That makes sense, looking forward to seeing the change. Thanks for the repro and fix. I'll get this merged and released today |
|
FWIW, I took a look at your branch and it looks more like a bugfix to me.
Yes, technically this is a break (as are most bug fixes, as they change behaviour), but the test I would usually apply is:
I think it's still a bit surprising that the afterEach runs at the start of the state teardowns rather than after all of them, but it's hard to do that with Pact-JS's proxy. Really, the afterEach should be first class options run by the pact core, so all languages get them, and the core can reason about the interactions with the teardowns too. This was one of the design philosophies I took when designing ContractCase - which won't work for you here, as it doesn't have afterEach / beforeEach methods at all yet. |
You might be able to count the state setup calls, and rely on the same number of teardowns, maybe. |
|
Thanks both. I'll take another look at the fix with those suggestion in mind. |
|
Opened #1243 which takes your comments into account. |
Thanks for this, I agree with the comments above - I wouldn't consider this a breaking change. It's incidentally working this way currently, and for most people this is unexpected behaviour. Thanks for merging Yousaf, I wanted to test myself before merging as middleware can be hard to reason about by just looking at the code and it's not an area that is well covered by tests. I was concerned it
I agree @TimothyJones and created this issue to track that: pact-foundation/pact-reference#468 (I thought I had previously done this, but it seems like it was a figment of my imagination). |
Currently a asynchronous afterEach hook will execute concurrently with the underlying express request lifecycle.
This fix allows async hooks to complete before allowing the express request to continue through its middleware stack.
Resolves #1241