experimental/websockets: support code and reason for close() and close event (cont'd) #5376
Conversation
etodanik
requested review from
AgnesToulet and
mstoykov
and removed request for
a team
|
@mstoykov This one should be addressing all of your change requests |
etodanik
temporarily deployed
to
azure-trusted-signing
GitHub Actions
Inactive
etodanik
temporarily deployed
to
azure-trusted-signing
GitHub Actions
Inactive
|
Will fix lint, but what's the failing test r.now? Doesn't replicate locally & doesn't look like it's directly connected to this PR's change. Am I missing something on it? @mstoykov |
|
THe browser tests are flaky so you shouldn't worry about that part - yes |
|
@mstoykov lol, the linter issue was literally just a rogue semicolon (C fingers...). |
etodanik
temporarily deployed
to
azure-trusted-signing
GitHub Actions
Inactive
etodanik
temporarily deployed
to
azure-trusted-signing
GitHub Actions
Inactive
|
If I'm not mistaken, the only failing tests are the flaky unrelated ones. |
etodanik
force-pushed
the
fix/ws-support-code-reason
branch
from
3e70b10 to
d061610
Compare
|
(rebased to latest) |
mstoykov
left a comment
mstoykov
left a comment
There was a problem hiding this comment.
Sorry for the slow reply, other PRs and priorities were taking way longer than anticipated for me.
LGTM in general! I have left some comments that are not really blocking, but it will be nice to do them!
| if errList := w.callErrorListeners(err); errList != nil { | ||
| return errList | ||
| } |
There was a problem hiding this comment.
maybe lets still have the TODO
| // needs to be called on the event loop | ||
| // TODO: move to events | ||
| func (w *webSocket) newEvent(eventType string, t time.Time) *sobek.Object { | ||
| func (w *webSocket) newEvent(eventType string, t time.Time, extras ...func(*sobek.Object)) *sobek.Object { |
There was a problem hiding this comment.
| func (w *webSocket) newEvent(eventType string, t time.Time, extras ...func(*sobek.Object)) *sobek.Object { | |
| func (w *webSocket) newEvent(eventType string, t time.Time, funcOptions ...func(*sobek.Object)) *sobek.Object { |
let's have a better name :)
| for _, listener := range w.eventListeners.all(eventType) { | ||
| // TODO the event here needs to be different and have an error (figure out it was for the close listeners) | ||
| if _, err := listener(w.newEvent(eventType, time.Now())); err != nil { // TODO fix timestamp | ||
| if _, err := listener(event); err != nil { |
There was a problem hiding this comment.
I can not figure out if those 2 TODOs are still relevant.
| if _, _, err := conn.ReadMessage(); err != nil { | ||
| once.Do(func() { close(closeReceived) }) |
There was a problem hiding this comment.
This changesr still seem irrelevant 🤔 I have no failures without them.
2 participants
What?
A re-opening of #4989 (feel free to close this and reopen that one if preferred)
Why?
I've addressed the requested changes.
Checklist
make check) and all pass.