search

fastify / fastify-websocket

basic websocket support for fastify
MIT License
394 stars 72 forks source link

Add preClose opt + merge close and preClose hook #261

Closed HDegroote closed 1 year ago

HDegroote commented 1 year ago

WARNING: current master has a failing test. To get past the precommit script, I had to comment it out. Let me know if you would like a different approach. The test in question (Should not hijack reply for an WS request to a WS route that gets sent a normal HTTP response in a hook) is failing on my local machine, where only 5 of the 6 planned verifications happen, but passes fine in the CI, so it does not affect this PR

All other tests (including the newly added ones) pass.

Note: I did not find a benchmark script to run as per the instructions below, so I did not check that box.

Fixes https://github.com/fastify/fastify-websocket/issues/260

Checklist

HDegroote commented 1 year ago

Sorry, had not realised the CI in man was broken because of something completely unrelated. The test does keep failing me as is though, both in main and in this branch. it works if I set t.plan(5) to t.plan(6), but I don't understand the test sufficiently well to know if that makes sense

HDegroote commented 1 year ago

As a sanity check, could you see if the tests of current master pass on your machine?

HDegroote commented 1 year ago

Added the types. Haven't worked with Typescript before, so best to sanity check. Noticed there were tests on the types, should I add something there too?

HDegroote commented 1 year ago

Reset that odd test to plan(6) again, as it was. I have a feeling this will pass the CI, because the previous run failed exactly on that test because there were too many assertions. Do note that I had to force this through without precommit check, because it fails for me. Could be something off on my end, but could also be that the test is a bit odd.

HDegroote commented 1 year ago

Great, anything else needed for this to be merged?