Skip to content
This repository has been archived by the owner on Jan 20, 2020. It is now read-only.

Emit error when websocket message is an error #221

Merged
merged 2 commits into from
Jan 18, 2018

Conversation

rmm5t
Copy link
Contributor

@rmm5t rmm5t commented Jan 11, 2018

What does it do?

Before, if an error message is sent from the server, that error is emitted as regular message data. Now, if a message of type=error arrives, it is emitted as an error instead.

What else?

Also removes auto-incrementing port for testing websocket server connections. We got to a point where we ran into port conflicts on the CI server.

Related Issues

/cc @fb55

@fb55
Copy link
Contributor

fb55 commented Jan 18, 2018

How do you feel about emitting an event with the type as the name as well? So instead of creating a special case for error just also create matches, open, l2update etc. events.

@rmm5t
Copy link
Contributor Author

rmm5t commented Jan 18, 2018

How do you feel about emitting an event with the type as the name as well? So instead of creating a special case for error just also create matches, open, l2update etc. events.

🤔 My initial gut reaction was to say no, but after considering how simple the implementation would be (simpler than this PR's changes), and as long as the generic message event is also emitted at the same time, I don’t really have any strong arguments that I can articulate at the moment.

Long story short, as long as message events remain for backwards compatibility and generic behavior, I think specific event types could be a nicety.

I'll modify this PR to be more generic like that.

@rmm5t
Copy link
Contributor Author

rmm5t commented Jan 18, 2018

@fb55 Oh. Here's an argument against the specfic event types. What about the websocket's open events (for Orders)? This would conflict with the socket's open events.

@fb55 fb55 merged commit 3acf6f5 into coinbase:master Jan 18, 2018
@fb55
Copy link
Contributor

fb55 commented Jan 18, 2018

Whee, good point. Merging as is, thanks a lot :)

@rmm5t rmm5t deleted the 219-promote-websocket-message-errors branch January 18, 2018 15:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants