Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Emit event when an websocket error occurs. #341

Merged
merged 1 commit into from Mar 22, 2018

Conversation

damour
Copy link
Contributor

@damour damour commented Jan 17, 2018

Use case:
Writing simple (load)testing tools. At that moment there is no simple way to determinate connection errors. Also useful to debug network troubles.

Could this be a plugin?
No, we can't subscribe to ws error event directly.

Is there a workaround?
Save connection state(to check that connection closed by server) and catch disconnected event. But we don't have any error details.

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change

@apollo-cla
Copy link

@damour: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@damour damour force-pushed the feature/websocket_error_event branch 2 times, most recently from 519b3fb to 6b505b2 Compare January 17, 2018 16:04
@tim-soft
Copy link

Will this allow logging the errors with something like apollo-link-error?

@timsuchanek
Copy link
Contributor

@tim-soft yes I guess so! Can this PR please taken care of? This is pretty important as right now there is no possibility to catch Websocket errors using the subscriptions-transport-ws client.

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! 😍

Two small nitpicks and a rebase and this would be ready to ship!

README.md Outdated
@@ -255,7 +255,7 @@ ReactDOM.render(
#### `unsubscribeAll() => void` - unsubscribes from all active subscriptions.

#### `on(eventName, callback, thisContext) => Function`
- `eventName: string`: the name of the event, available events are: `connecting`, `connected`, `reconnecting`, `reconnected` and `disconnected`
- `eventName: string`: the name of the event, available events are: `connecting`, `connected`, `reconnecting`, `reconnected`, `disconnected` and `websocket_error`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this event error instead, no need to specify websocket just like in the other events.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed.

README.md Outdated
@@ -285,6 +285,11 @@ ReactDOM.render(
- `thisContext: any`: `this` context to use when calling the callback function.
- => Returns an `off` method to cancel the event subscription.

#### `onWebSocketError(callback, thisContext) => Function` - shorthand for `.on('websocket_error', ...)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, no need for the WebSocket, let's call this onError.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@damour damour force-pushed the feature/websocket_error_event branch from 6b505b2 to 1a3d0e6 Compare March 22, 2018 09:22
@damour damour force-pushed the feature/websocket_error_event branch from 1a3d0e6 to c46c5d8 Compare March 22, 2018 09:23
Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@mxstbr mxstbr merged commit 54a8100 into apollographql:master Mar 22, 2018
baconz pushed a commit to PhiloInc/subscriptions-transport-ws that referenced this pull request Apr 24, 2018
…types/sinon-2.1.0

Update @types/sinon to the latest version 🚀
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants