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.

Sign up for GitHub

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

Jump to bottom

Update WebSocket examples #713

Merged
merged 2 commits into from Mar 29, 2017
Merged

Update WebSocket examples #713

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9bcadc
Update WebSocket examples
domenic Mar 28, 2017
d3286a4
Attempt to avoid close + error collisions
domenic Mar 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion index.bs
View file
Open in desktop
Expand Up @@ -4230,7 +4230,10 @@ promises returned by its <a lt="writable stream writer">writer</a>'s {{WritableS

return new WritableStream({
start(controller) {
ws.onerror = () => controller.error(new Error("The WebSocket errored!"));
ws.onerror = () => {
controller.error(new Error("The WebSocket errored!"));
ws.onclose = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

That should work.

Copy link
Member Author

Choose a reason for hiding this comment

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

So does this overall LGTY now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. lgtm.

};
ws.onclose = () => controller.error(new Error("The server closed the connection unexpectedly!"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

If for example the remote server closes the connection abruptly, then onerror and onclose will both fire.

I don't actually know of a case where onerror fires without onclose also firing. Certainly Chrome's implementation never does it. So one way of simplifying this would just be to not set an onerror handler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to check my understanding, the server could close it non-abruptly, right? In which case close would fire but not error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, so this definitely needs to be fixed since you can't call controller.error() twice. Not sure about the best way to do that while balancing:

  • Giving as much information to the developer as possible (e.g. bad close vs good)
  • Keeping the code simple enough for an example

Will noodle on it.

return new Promise(resolve => ws.onopen = resolve);
},
Expand Down Expand Up @@ -4386,6 +4389,7 @@ source abstractions.
this._ws.onclose = () => controller.error(new Error("The server closed the connection unexpectedly!"));
this._ws.addEventListener("error", () => {
controller.error(new Error("The WebSocket errored!"));
this._ws.onclose = null;
});

return new Promise(resolve => this._ws.onopen = resolve);
Expand Down