Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log socket close error if any #2108

Merged
merged 1 commit into from
Feb 27, 2018
Merged

Log socket close error if any #2108

merged 1 commit into from
Feb 27, 2018

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Feb 23, 2018

23-180513388
23-180652966

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Feb 23, 2018
@xPaw xPaw added this to the 3.0.0 milestone Feb 23, 2018
@astorije
Copy link
Member

Errors are not really human-friendly here. Like I almost don't even know what they mean.
What about a nice message like "The connection with the server was lost", and keep the detailed error hidden like we did in that other PR?

@xPaw
Copy link
Member Author

xPaw commented Feb 24, 2018

@astorije Hiding the error makes that entire message unnecessary (there's a separate disconnected event).

While these errors can be cryptic, they can be searched for further information rather easily so it gives some context.

Chrome for example shows the error code too sometimes:

24-065014161

@astorije
Copy link
Member

Well yeah, but like your screenshot shows, human-friendly messages are in bigger, bolder fonts, and the specific error code is muted below.

I just think we can do better error reporting than this is doing, and in general across our project, but I'm not going to push hard for my proposition above.

At the very least if you want to keep the detailed message on screen (which is now inconsistent with prefetch error message, but again our error reporting across the project is not really great anyway), it would be great if we could go from "Socket closed: [...]" into something more digestible like "The connection with the server was lost: [...]". Open to better wording of course :)

@xPaw
Copy link
Member Author

xPaw commented Feb 26, 2018

@astorije I changed it to Connection closed unexpectedly

@xPaw xPaw merged commit 92eb45a into master Feb 27, 2018
@xPaw xPaw deleted the xpaw/socket-closed branch February 27, 2018 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants