Buffer support for Node request body #33
Conversation
|
Looks good to me 👌🏻 To get rid of the effect you can use |
|
Right, I forgot about ByteString. By the way, do you any plans on making it portable across PureScript backends? Then we could use it more in the core parts/APIs, if needed (basically like WAI depends on ByteString in Haskell). |
|
No concrete plans but open to the idea. Shouldn't be difficult except the dependency on node-buffers. |
src/Hyper/Node/Server.purs
Outdated
| chunks <- makeVar' [] | ||
| res <- liftEff $ | ||
| catchException (pure <<< Left) (Right <$> fillBody stream chunks completeBody) | ||
| either throwError (const (takeVar completeBody)) res | ||
| where | ||
| fillBody stream chunks completeBody = do |
There was a problem hiding this comment.
This part catches the EXCEPTION effect from Stream.onData (which seems to be thrown if the Node stream emits a Chunk that is not a String or a Buffer.) How that relates to Stream.onError, I'm not sure, but we should perhaps handle the onError case as well.
There was a problem hiding this comment.
The Stream API is so messy... 😞
There was a problem hiding this comment.
It would be nice to wrap purescript-node-streams with purescript-aff-coroutines. It should not be very difficult. The coroutine could emit (Either Error Buffer), and it could then be wrapped by another coroutine that uses ExceptT and emits Buffer.
There was a problem hiding this comment.
I'll check that out. Maybe do that as a separate PR?
There was a problem hiding this comment.
Sounds good, then we can easily abandon it if it doesn't work out.
|
@rightfold added |
|
Sounds good 👍🏻 |
@rightfold: Thought I'd break the changes we discussed in #11 up into smaller PRs, so here's just the Buffer support. The
Stringinstance we had before now uses the samereadBodyAsBufferas theBufferinstance, which means theBUFFEReffect spreads all over in existing code usingreadBody. 😞 I'm OK with that for now, though. We can provide some effect alias type for convenience.I'm thankful for any feedback!