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.
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
Editorial review: Document WebSocketStream #35548
Editorial review: Document WebSocketStream #35548
Changes from 17 commits
0440e77
fb8a621
54ab5af
3c8f355
6ca71c9
7a55d73
8f60ee6
a979ad4
1016e40
f2e04e4
2c01603
76923cc
71defc9
2a82373
190b3da
bbb9b37
f72d08d
2c2ceed
7bc83c3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not necessarily event-driven)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable. Updated to use your wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I like this a lot better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also experimental?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, makes sense. I've also added it to the
WebSocketStream
interface pages.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW this should get added automatically from the BCD. But only in reference pages, not in guide pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK. I keep forgetting what is automatically added and what isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think is where we should spell out the backpressure thing. Perhaps something like:
(assuming that is true!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the point when
read()
is called has anything to do with backpressure. This is not automatic, but a choice made by the developer in the code?I've read around this subject a bit, and added the following paragraph after the code snippet:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(please double check the syntax for the H2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh darn, thanks for the fixes there.
The syntax worked, but I ended up replacing the h2 macro call with a regular link, as for some reason the link was not showing the angle brackets or in code font.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the server sends
"<script src="https://evil.org/steal-your-stuff.js"></script>"
? Is this a concern?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be, yes, but this is a general security concern, not something specific to WebSockets. It would be the same if you somehow fetched a code snippet with something malicious in it? It is of course up to the developer to make sure they connect to a trusted server, have CSP or other mechanisms in place to prevent unwanted scripts running, etc.
I'm not convinced I need to include such details on this page. What do you think should be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking, use
textContent
or something else safe, instead ofinsertAdjacentHTML
.The issue about a trusted server is, how do you know you can trust it, and it hasn't been compromised? As for CSP, guidance I've read is that it is not a substitute for not removing XSS vectors.
It seems inherently unsafe to connect to a server expecting it to send you messages, then immediately (effectively) executing them without sanitizing them. And by putting this in an example we're telling devs that this is an OK thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, and I would make the same comment about a
fetch()
example that did this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this makes sense — I am not sure why I used
insertAdjacentHTML()
. I will update it to use textContent, which is what I normally use for such examples ;-\There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why await? What happens if we don't? What difference does it make if we do? One thing is that error cases throw, but we don't have a try/catch handler for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's promise-based, so it won't work without it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to await to make it work: await just ensures that the promise fulfills before it executes the next line.
I mean, usually I see
await
being used if you want to make your code behave like synchronous code, for instance if it's returning something you need in the next line:(or as we are doing with
read()
, where we need the read value in the next line).But in this case we're not, so what's functionally the difference between awaiting and not awaiting, except that we're imposing an ordering when we maybe don't need to.
I suppose awaiting means we can report
writeToScreen("SENT: ping");
, because we knowwrite()
didn't reject, although it's not clear whether not rejecting actually means we sent it, as https://developer.mozilla.org/en-US/docs/Web/API/WritableStreamDefaultWriter/write says:But then if that is the reason, isn't it better to use try/catch so we can report an error?
So it could say:
...and then there's a use for
await
.Ah, maybe it's not worth worrying about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is really interesting. I've been using this stuff for years and not considered that before. I've learnt something today.
I thought about it a bit and decided to just remove the unneeded
await
keywords (and theasync
keyword on the anonymous function inside thesetTimeout()
call). I don't want to add in a bunch of error handling code just cos. It doesn't add much to what I am trying to demonstrate, and increases complexity.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I've actually updated the
write()
call inside thesetTimeout()
function to use the try/catch structure you suggested because sometimes you close the connection and then the app still tries to write to the stream afterwards, causing an error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear: I'm not certain what the right thing to do here is, which is why I'm asking this as questions. I think async can be complex, especially when there's error-handling going on, and I don't have the time/bandwidth to say what this code ought to be doing. I just want us to think carefully about why we're making the choices that we are here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We say a few times that it's preferable to use
abort()
, maybe better to show that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also say that close() is used when providing a custom code and/or reason. In this case we are providing a custom reason, so I think this is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be pedantic, which I always am, it says "custom code and reason".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, all relevant instances now changed to "and/or".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be easier to read this if the JS were in an external script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly so, but this would also make it slightly more annoying for a user to copy and paste and chuck into a file for testing purposes. I think it is better to leave as-is, by this token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call; updated.