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

retrun json "null" if ctx.type is set to json and ctx.body is set to … #1059

Closed
wants to merge 2 commits into from

Conversation

likegun
Copy link
Contributor

@likegun likegun commented Sep 14, 2017

issue 998

@codecov
Copy link

codecov bot commented Sep 14, 2017

Codecov Report

Merging #1059 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1059      +/-   ##
==========================================
+ Coverage   99.73%   99.73%   +<.01%     
==========================================
  Files           5        5              
  Lines         374      376       +2     
==========================================
+ Hits          373      375       +2     
  Misses          1        1
Impacted Files Coverage Δ
lib/response.js 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 148f26f...b21f526. Read the comment docs.

Copy link
Contributor

@fl0w fl0w left a comment

Choose a reason for hiding this comment

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

Nitpicking

lib/response.js Outdated
@@ -137,7 +137,10 @@ module.exports = {

// no content
if (null == val) {
if (!statuses.empty[this.status]) this.status = 204;
if(!statuses.empty[this.status]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: space between if and parenthesis.

@jonathanong the reason this passes tests is because --fix is automatically run on travis, and so it'll fix it but the commit remains "invalid" according to eslintrc.yml.

lib/response.js Outdated
@@ -137,7 +137,10 @@ module.exports = {

// no content
if (null == val) {
if (!statuses.empty[this.status]) this.status = 204;
if(!statuses.empty[this.status]) {
if(this.type == 'application/json') return this._body = 'null';
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@likegun
Copy link
Contributor Author

likegun commented Sep 18, 2017

@fl0w done

@dead-horse
Copy link
Member

IMO it is a breaking change, and we have to hold this PR until we plan to develop koa@3.x.

@damianobarbati
Copy link

@dead-horse do you have any approximate schedule for the 3.0 to ship this within?

@jonathanong
Copy link
Member

@damianobarbati there's no schedule. if we get another PRs to justify a v3, we should push.

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

Successfully merging this pull request may close these issues.

5 participants