feat(deps): update fastify-websocket to 1.x.x, fix tests #96
Conversation
This file contains 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
mcollina
requested changes
Jan 2, 2020
lib/subscription-connection.js
Outdated
| @@ -123,6 +123,7 @@ module.exports = class SubscriptionConnection { | |||
| payload | |||
| })) | |||
| } catch (e) { | |||
| /* istanbul ignore next */ | |||
There was a problem hiding this comment.
This was covered before, why it is not anymore? Maybe we should add a new test.
There was a problem hiding this comment.
It was covered because of server tried to send data to already closed socket. And as far as I understood it was triggered just occasionally as side-effect of some test.
I've added one test, but I'd rather refactor send message method slightly:
- Check if socket is open before trying to send anything to it. Don't know how it would impact on performance, but it seems better approach for me.
- Since it called in a loop calling
JSON.stringifyin it may result in overhead. I think it would be better to call it once when subscription message arrives and send method should accept only strings as messages. May be use fast-json-stringify in here, but also don't know if it will add some benefit.
In this case as bench shows fast-json-stringify is slower than JSON.stringify.
But I think this would be in other PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Update fastify-websockets to 1.x.x and fix tests.