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

Rework part of the lib to be more versatile #31

Merged
merged 1 commit into from
Apr 16, 2021
Merged

Rework part of the lib to be more versatile #31

merged 1 commit into from
Apr 16, 2021

Conversation

Avaq
Copy link
Member

@Avaq Avaq commented Apr 13, 2021

This introduces some ideas that @voxbono and I worked out on Gitter a while ago. You might be interested @voxbono :)

The idea is that the constructors themselves don't take any headers or status code, but that they can be added via function calls afterwards:

Json ({me: 'teapot'})
|> withStatus (418)
|> withHeader ('X-Powered-By') ('230 Volts')

index.js Outdated
Comment on lines 242 to 318
//# withHeader :: String -> String -> Response a b -> Response a b
//.
//. Sets the value of a header on the Response.
export const withHeader = key => value => cata ({
Respond: (headers, code, body) => Response.Respond (
code,
{...headers, [key.toLowerCase ()]: value},
body,
),
Next: Response.Next,
});

//# withoutHeader :: String -> Response a b -> Response a b
//.
//. Removes a header from the Response.
export const withoutHeader = key => cata ({
Respond: (headers, code, body) => Response.Respond (
code,
{...headers, [key.toLowerCase ()]: undefined},
body,
),
Next: Response.Next,
});
Copy link
Member Author

Choose a reason for hiding this comment

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

Before this PR, it was not possible to modify response headers through fluture-express

index.js Outdated
Comment on lines 266 to 408
//# withStatus :: Number -> Response a b -> Response a b
//.
//. Sets the status code of the Response.
export const withStatus = code => cata ({
Respond: (headers, _, body) => Response.Respond (headers, code, body),
Next: Response.Next,
});
Copy link
Member Author

Choose a reason for hiding this comment

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

Allowing response status to be modified allows Response constructors to use default response codes, making the common use case a little less verbose, and the less common use case a little more verbose.

@Avaq
Copy link
Member Author

Avaq commented Apr 13, 2021

I might want to add convenient ways of adding to the Cookies header, and the Content-Type header.

@Avaq Avaq added the breaking label Apr 13, 2021
@Avaq Avaq self-assigned this Apr 13, 2021
@Avaq
Copy link
Member Author

Avaq commented Apr 13, 2021

And possibly also withMessage to override the response message.

@dicearr
Copy link
Contributor

dicearr commented Apr 14, 2021

I like this new approach.

I might want to add convenient ways of adding to the Cookies header, and the Content-Type header.

I am curious about this one. Why would you do so? Are those headers special in any way?

@Avaq
Copy link
Member Author

Avaq commented Apr 14, 2021

@dicearr See the new commit. The idea is to have this lib be a straight-up proxy for the various methods and convenience methods that Express gives users to modify the response. So this lib will concern itself more with Express' interface, and less with what an HTTP response means in general.

@Avaq Avaq changed the title Rework part of lib to be more versatile Rework part of the lib to be more versatile Apr 14, 2021
@Avaq Avaq marked this pull request as ready for review April 14, 2021 12:27
@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #31 (24a7f30) into master (f83c729) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 24a7f30 differs from pull request most recent head 6af90da. Consider uploading reports for the commit 6af90da to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            master       #31    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            1         1            
  Lines          239       461   +222     
==========================================
+ Hits           239       461   +222     
Impacted Files Coverage Δ
index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f83c729...6af90da. Read the comment docs.

Copy link
Contributor

@dicearr dicearr left a comment

Choose a reason for hiding this comment

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

Looks great.

It has taken me quite some time to wrap my head around the deriveEq function but I couldn't come up with a simpler/more obvious approach.

index.js Outdated Show resolved Hide resolved
@Avaq Avaq merged commit f47a825 into master Apr 16, 2021
@Avaq Avaq deleted the avaq/versatile branch April 16, 2021 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants