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

feat(core): handle response content types #37

Merged
merged 8 commits into from
May 29, 2018

Conversation

JozefFlakus
Copy link
Member

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[x] Tests

What is the current behavior?

  • When trying to serve static files, responseHandler tries to JSON.stringify() every outgoing response.
  • responseHandler doesn't detect mime-types automatically

Issue Number: #33

What is the new behavior?

  • stream based fileReader
  • integration tests for serving static files
  • auto set response content-type based on provided body
  • moved marbles.spec-util to common util folder
  • added mock-fs, mime and file-type packages

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@JozefFlakus JozefFlakus added bug Something isn't working enhancement New feature or request labels May 29, 2018
@JozefFlakus JozefFlakus added this to the v0.3.2 milestone May 29, 2018
@JozefFlakus JozefFlakus self-assigned this May 29, 2018
@JozefFlakus JozefFlakus requested a review from a team May 29, 2018 17:47
@codecov
Copy link

codecov bot commented May 29, 2018

Codecov Report

Merging #37 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #37   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          21     24    +3     
  Lines         230    283   +53     
  Branches       24     31    +7     
=====================================
+ Hits          230    283   +53
Impacted Files Coverage Δ
...kages/core/src/response/responseHeaders.factory.ts 100% <100%> (ø) ⬆️
packages/core/src/response/responseBody.factory.ts 100% <100%> (ø)
...s/core/src/response/responseContentType.factory.ts 100% <100%> (ø)
packages/core/src/util/contentType.util.ts 100% <100%> (ø) ⬆️
packages/core/src/http.listener.ts 100% <100%> (ø) ⬆️
packages/util/fileReader.helper.ts 100% <100%> (ø)
packages/core/src/response/response.handler.ts 100% <100%> (ø) ⬆️

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 d406beb...aca92cc. Read the comment docs.

matchPath('/static/:dir'),
matchType('GET'),
map(req => req.params!.dir as string),
switchMap(readFile(__dirname + './../../docs/assets')),
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of putting this path in some const and maybe you could use path.resolve here :D ?

pdomaleczny
pdomaleczny previously approved these changes May 29, 2018
@JozefFlakus JozefFlakus merged commit fc9f09a into master May 29, 2018
@JozefFlakus JozefFlakus deleted the bugfix/response-content-type branch May 29, 2018 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants