Support timeout option #8
Conversation
mgreystone
requested review from
pklingem,
rpearce,
flintinatux,
evilsoft and
jsilve
client/index.js
Outdated
| sent = sent.bimap(cleanup, cleanup) | ||
| } | ||
|
|
||
| sent.fork(reject, resolve) |
There was a problem hiding this comment.
@evilsoft This feels rather clunky to me, & i'm sure i missed something. I'm particularly worried that i'm not handling cancelled asyncs correctly.
There was a problem hiding this comment.
Relevant/tangential/helpful reading on this sort of thing: https://mostly-adequate.gitbooks.io/mostly-adequate-guide/ch08.html#asynchronous-tasks
There was a problem hiding this comment.
In reading the docs, I'm not sure I do either
There was a problem hiding this comment.
Head is exploding. Currently watching https://www.youtube.com/watch?v=O1Gu5b7rxbw
There was a problem hiding this comment.
Yes it is clunky. But for what you have this will have to work.
I just put this issue in and will get it out ASAP so we can do non-clunky things.
There was a problem hiding this comment.
@evilsoft sorry for spam. Do you have a video / writeup on cancelling Asyncs manually? Not quite grokking it from the docs
There was a problem hiding this comment.
In crocks you cannot cancel an Async from the inside, breaks way too many laws.
BUT you can return a function that will be called when a fork is cancelled. When you call fork it will give you back a function. When that function is called it will cancel the forked flow. Will not resolve/reject and will not run an more Asyncs in a flow that are not already in flight or settled.
There was a problem hiding this comment.
I was definitely thinking of this way too mutably. This makes sense. Thank you
There was a problem hiding this comment.
Doing it the way we decided to in crocks keeps all side effects at the edge (as you call fork at the edge it makes sense for cancelling (like when some side effect pushes a cancel button, etc). So @mgreystone HAS to do it this way for his flow because he does not want to cancel but Reject after an amount of time.
There was a problem hiding this comment.
@evilsoft, should we be cancelling the inner sent Async, in addition to reject-ing the outer Async?
There was a problem hiding this comment.
I like the approach. I've written similar algorithms with Promises in the past. The one big question I've got is whether this is a good fit for sox, since it's not transport specific. We could do much of the same thing with a util function that accepts any Async, not just a sox one, and wraps it in another Async that handles the timeout. Seems like more of an managing-Asyncs thing than a sox thing.
client/index.js
Outdated
| @@ -64,6 +82,9 @@ const sox = (args = {}) => { | |||
| // send : String -> a -> Async Action | |||
| socket.send = send | |||
|
|
|||
| // send : Object -> String -> a -> Async Action | |||
There was a problem hiding this comment.
In the past we've used { k: v } in signatures, rather than Object. Following ramda a bit on that, I think.
client/index.js
Outdated
| sent = sent.bimap(cleanup, cleanup) | ||
| } | ||
|
|
||
| sent.fork(reject, resolve) |
There was a problem hiding this comment.
@evilsoft, should we be cancelling the inner sent Async, in addition to reject-ing the outer Async?
client/index.js
Outdated
| @@ -1,4 +1,5 @@ | |||
| const action = require('@articulate/ducks/lib/action') | |||
| const error = require('@articulate/ducks/lib/error') | |||
There was a problem hiding this comment.
Let's keep this list alphabetical.
|
@flintinatux , @evilsoft has a crocks PR that will move a lot of this cruft to the crocks lib. See evilsoft/crocks#282. I think that will address most of your separation of concerns.... concerns... Even so, i would argue that this is a sox feature. Because i don't just want to manage timeouts. I want to reject with the given action on timeout. That seems like a sox thing to me. |
|
@mgreystone, I'm ok with this approach then. Thanks for clarifying. |
|
@evilsoft @flintinatux @rpearce Crocks has been bumped (thank you!) & now using the new async race & rejectAfter functions. Ready for final review. |
them's mergin' words |
|
How incredibly simple this code has become. |
|
I dig it. Really nice @mgreystone |
Add a new
sendWithOptionsfunction that accepts a timeout option. Reject with an error action ifsenddoes not resolve before the timeout.This is intended to trigger an error when we expect an action from a socket but it never occurs, possibly because the service is down or there is a version mismatch between frontend & backend.