Replace http.STATUS_CODES with the statuses module #3215
Conversation
wesleytodd
force-pushed
the
statuses-module
branch
2 times, most recently
from
987423f to
d8c5930
Compare
test/res.redirect.js
Outdated
| @@ -104,7 +104,7 @@ describe('res', function(){ | |||
| .set('Accept', 'text/html') | |||
| .expect('Content-Type', /html/) | |||
| .expect('Location', 'http://google.com') | |||
| .expect(302, '<p>' + http.STATUS_CODES[302] + '. Redirecting to <a href="http://google.com">http://google.com</a></p>', done); | |||
| .expect(302, '<p>' + statuses[302] + '. Redirecting to <a href="http://google.com">http://google.com</a></p>', done) | |||
There was a problem hiding this comment.
Just hard-code the literal text here. It was dynamic because http was not consistent between major versions, but statuses will be consistent.
|
Ideally I think it can land on 4.x. The target of the PR itself is not too big of an issue, as we can merge it where ever, but if you want to update the PR to target |
wesleytodd
force-pushed
the
statuses-module
branch
from
d8c5930 to
461d080
Compare
wesleytodd
force-pushed
the
statuses-module
branch
from
461d080 to
efdb52f
Compare
|
Ok, made the test change, and updated to PR against |
lib/response.js
Outdated
| @@ -25,7 +25,7 @@ var sign = require('cookie-signature').sign; | |||
| var normalizeType = require('./utils').normalizeType; | |||
| var normalizeTypes = require('./utils').normalizeTypes; | |||
| var setCharset = require('./utils').setCharset; | |||
| var statusCodes = http.STATUS_CODES; | |||
| var statusCodes = require('statuses/codes') | |||
There was a problem hiding this comment.
I'm iffy about requiring any sub file in a module. The statuses API is lame, but probably can still be used here. I would just do var statuses = require('statuses') and then the code below would still work, after the variable name change (with a possible oddity in res.sendStatus that would need to be dealt with regardless of this change).
There was a problem hiding this comment.
Hm, maybe I am missing something here, but the object found in statuses/codes is not directly exposed. So I don't think I can get Found from 302 in the existing api. I would either need to change the statuses package to expose it, or use the statuses.codes array to reverse engineer back to the original data structure. Of those two, I obviously lean toward updating the statuses package to expose the raw codes object.
There was a problem hiding this comment.
$ node -pe 'require("statuses")[302]'
Found
There was a problem hiding this comment.
Lol, yeah totally missed the mechanism that exposed that on the first read. Sorry for the bother. Updating now.
wesleytodd
force-pushed
the
statuses-module
branch
from
efdb52f to
0665a78
Compare
Replaced usage of core
http.STATUS_CODESwith the list from thestatusesmodule. This was pulled out from #3213 as recommended by @dougwilson in a side conversation. For posterity, it is known that it might be considered a "breaking" change because it changes the response text, which is why the tests were breaking.@dougwilson I couldn't remember if you said to make the PR against 5.0 and backport it, or against 4.0 and cherry-pick it forward, I know we mentioned both ways. Let me know if I did this the wrong way and I can fix it.