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

Add Data responses and TypedDataResponse #1657

Merged
merged 4 commits into from May 8, 2018

Conversation

4 participants
@aydenp
Copy link
Contributor

aydenp commented May 5, 2018

In some cases, you may want to just return raw Data as a Response. Before this commit, you might have had to make your own ResponseEncodable type that just provides the data, which seems a bit much for such a simple function.

Say we have this super important data that we need to respond with:

let data = "Hello!".data(using: .utf8)!

At it's simplest, this new functionality allows you to respond with a Data object by simply returning it:

router.get("hello") { req in
    return data
}

However, sometimes, you might want to return this data with a custom content type. In this case, you can simply get a TypedDataResponse from a new Data extension, and return it, instead of the Data:

router.get("hello") { req in
    return data.response(type: .html)
}

As an aside, TypedDataResponse is also used under the hood if you just return Data on it's own, with a content type of MediaType.any.

Checklist

  • Circle CI is passing (code compiles and passes tests).
  • There are no breaking changes to public API.
  • New test cases have been added where appropriate.
  • All new code has been commented with doc blocks ///.

@aydenp aydenp changed the title Add data responses and TypedDataResponse Add Data responses and TypedDataResponse May 7, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

Just some gardening comments. Looks great I love the idea.

@@ -0,0 +1,30 @@
/// A container representing `Data` with a specific content type (used for returning as a `Response`).
public struct TypedDataResponse: ResponseEncodable {

This comment has been minimized.

@tanner0101

tanner0101 May 8, 2018

Member

We should make this type private and just publicize ResponseEncodable.

This comment has been minimized.

@aydenp

aydenp May 8, 2018

Author Contributor

ResponseEncodable is already public, but I agree that this type should be private. An earlier iteration of it didn't have the Data extension for types, so I'd forgotten to make it private.

Get a `ResponseEncodable` container holding this data with your provided contentType.
- parameter contentType: The type of data to return the container with.
*/
public func response(type: MediaType) -> TypedDataResponse {

This comment has been minimized.

@tanner0101

tanner0101 May 8, 2018

Member

Here we should be able to just return ResponseEncodable.

public struct TypedDataResponse: ResponseEncodable {
/// The data held by this container.
let data: Data
/// The type of the data held by this container.

This comment has been minimized.

@tanner0101

tanner0101 May 8, 2018

Member

should be a newline above this comment.

}

/**
Get a `ResponseEncodable` container holding this data with your provided contentType.

This comment has been minimized.

@tanner0101

tanner0101 May 8, 2018

Member

couple gardening notes here:

  • vapor uses /// format comments
  • - parameters should be listed as a separate bulleted list, see any of the other doc blocks for an example
  • methods expected to be used directly by the end user should have a small example code section if possible! that really helps people figure out how to use it.

@tanner0101 tanner0101 self-assigned this May 8, 2018

aydenp and others added some commits May 8, 2018

@tanner0101 tanner0101 added this to the 3.0.2 milestone May 8, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

I will fix the test errors on a different branch. thanks!

@tanner0101 tanner0101 merged commit 199593a into vapor:master May 8, 2018

0 of 3 checks passed

ci/circleci: linux CircleCI is running your tests
Details
ci/circleci: linux-api-template CircleCI is running your tests
Details
ci/circleci: linux-release Your tests are queued behind your running builds
Details
@penny-coin

This comment has been minimized.

Copy link

penny-coin commented May 8, 2018

Hey @aydenp, you just merged a pull request, have a coin!

You now have 1 coins.

tanner0101 added a commit that referenced this pull request May 8, 2018

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented May 9, 2018

Ended on something slightly different, but I think it better matches the other available APIs which will make things more intuitive.

// default to plaintext
router.get("hello") { req in
    return req.makeResponse("Hello!")
}

// specify content type
router.get("hello-html") { req in
    return req.makeResponse("Hey!", as: .html)
}

// and of course, what we had previously
router.get("hello-json") { req in
    let res = req.makeResponse()
    try res.content.encode(["hello": "world"], as: .json)
    return res
}
@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented May 9, 2018

Worth noting that both String and Data conform to HTTPBodyLosslessRepresentable. Any other types that conform can also be used with this new method.

@nullpixel

This comment has been minimized.

Copy link

nullpixel commented May 9, 2018

Do your changes let you just return Data, like return someData?

If they don't, then I'd suggest this is less initiative.

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented May 9, 2018

@nullpixel what Content-Type would Data use? There wasn't an obvious best choice there which is why I omitted that part.

@nullpixel

This comment has been minimized.

Copy link

nullpixel commented May 9, 2018

Well, .any makes the most sense, since, well, it can be anything. But you do have a valid point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment