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.

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

Chunked responses #107

Merged
merged 6 commits into from
Aug 30, 2018
Merged

Chunked responses #107

merged 6 commits into from
Aug 30, 2018

Conversation

cprussin
Copy link
Collaborator

This is the first part of #63 (we'll need to also provide better support around chunked requests to close that ticket).

In this PR, I've also moved away from the size function on the Body typeclass in favor of the much more generic additionalHeaders function. Feels like a good move to me, let me know your thoughts.

There's also a bit of general cleanup in here--I've renamed the Image example to Binary to better reflect what it's illustrating, and a few other random things.

@cprussin cprussin requested a review from akheron August 27, 2018 19:23
@@ -17,45 +17,67 @@ import Node.Encoding as Encoding
import Node.HTTP as HTTP
import Node.Stream as Stream

import HTTPure.Headers as Headers

newtype Chunked = Chunked (Stream.Readable ())
Copy link
Collaborator Author

@cprussin cprussin Aug 27, 2018

Choose a reason for hiding this comment

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

We have to use a newtype here, because typeclass instances don't support row definitions in their heads, and Stream.Readable is just a type synonym for Stream.Stream (read :: Stream.Read | s). I don't know that purescript provides any workaround, I've heard tell that there's a ~ operator that can be used to get around this but I'm not sure if it's actually landed and haven't been able to find any docs.

Reference: purescript/purescript#2196

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was just thinking, as another approach, instead of this, we work towards the API you mentioned in #104 (comment) and we could make a typeclass instance for something like this:

(String -> Effect.Effect Unit) -> Aff.Aff Unit

That would lead to an API that looks more like what you mentioned:

router _ = HTTPure.ok \writeData -> do
    writeData "hello"
    writeData "world"

I'd want to figure out some clean API around sending strings or binary data with this if we go this route.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In my opinion there should be an API for both streams and callback functions.

Stream API will be useful when dealing with files on disk, and it could just use pump to copy the input stream to the response.

You can declare separate typeclass instances for functions with different parameter types, so it would be possible to support callback functions that generate strings and callback functions that generate binary chunks without newtype wrappers. (Although I'm not sure whether it would be actually good to have newtype wrappers in this case, to get better type mismatch errors.)

The best possible signatures for the callback functions need to be worked out. There might e.g. be some state that the callback function needs to update, and supporting this would be nice without the user having to resort to Effect.Ref.

Copy link
Collaborator

Choose a reason for hiding this comment

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

... and it might be best to leave defining the callback API for later when there's actually a real use case for it. At least I don't have a use case in mind at the moment, the only thing I need is sane serving of static files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I agree, both is good and I can revisit the callback API later. I would like to implement it before we release 1.0, but I'm fine with doing another minor release without it in the meantime.

Also, per purescript/purescript#1510, there is after all a way to declare the typeclass for the stream without the newtype wrapper, so I'll get that change done in this PR.


-- | Return a readable stream that emits the first string, then the second
-- | string, with a delay in between given by the third argument
foreign import stagger :: String -> String -> Int -> Stream.Readable ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Coming originally from Elm, I cringe on examples (or any code) that require FFI. Would it make this example harder to comprehend or its point unclearer to e.g. read a file or run an external process to get the streaming response body?

Copy link
Collaborator Author

@cprussin cprussin Aug 28, 2018

Choose a reason for hiding this comment

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

So, I totally 100% agree with you, and was really hesitant to do this. The reason I decided on this instead of reading a file were:

  1. Creating a custom stream is something that purescript-node-streams should probably support. I'm waiting for a response to support custom stream creation purescript-node/purescript-node-streams#19; if they accept a PR to add custom streams to the API (since it's part of the Node API, I expect that they will) then we'll be able to get rid of the FFI code and go full native.

  2. I didn't want to use files as the example here, because once the file helpers are in place we will want folks to bias towards those over using the Stream API. Having an example that directly contradicts that will be confusing (this is probably true of the Binary example as well; once the file helpers are in place, I'd like to revisit that example and use something that isn't a file).

Those points said, I think a nice compromise could be using an external process. I'll definitely look into making that change.

size :: b -> Effect.Effect (Maybe.Maybe Int)
-- | Return any additional headers that need to be sent with this body type.
-- | Things like `Content-Type`, `Content-Length`, and `Transfer-Encoding`.
additionalHeaders :: b -> Effect.Effect Headers.Headers
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be a nice addition to document somewhere whether the user can override these additional headers by using e.g. ok', or are they applied after user defined headers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can do; in fact, they cannot be overridden with the current implementation but I should reverse that. For now, I'll document it here; I have a ticket open to revisit the guides (#106) before we release the next version and I'll make sure to add a note there about overriding default headers

@cprussin
Copy link
Collaborator Author

@akheron changes made PTAL when you get a chance, thanks

@cprussin
Copy link
Collaborator Author

@akheron going ahead and merging this so I can get moving on the file helpers; let me know if you find anything you think should be changed and I'll put in a follow-up PR

@cprussin cprussin merged commit 1adbcec into master Aug 30, 2018
@cprussin cprussin deleted the #63-streaming branch August 30, 2018 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants