feat(response): new setting strict status codes #5856
Conversation
|
I think we need to add a test that is outside the strict range -- I could not find any existing |
There was a problem hiding this comment.
I was under the impression we intended to turn this on by default. @jonchurch Wasn't that your intent previously?
aagamezl
force-pushed
the
aa/feat/strict-status-code
branch
from
61c44d7 to
f5a28b0
Compare
|
@ctcpip I added the missing test |
|
@wesleytodd maybe I misunderstood, could you confirm the default value of the setting? Default: true or |
|
I could be wrong, but I thought we had discussed with @jonchurch doing the strict checks by default in v5. I am not strongly opinionated either way, but if we dont turn it on by default now then we cannot until v6. |
aagamezl
force-pushed
the
aa/feat/strict-status-code
branch
from
f5a28b0 to
18331b2
Compare
|
@wesleytodd I've updated the code according to the conversation, now "strict status codes" is true by default. |
|
@wesleytodd, this isn't strictly required for v5 We had discussed adding this setting, and Blake suggested we err on the side of unopinionated and not set this by default. Which Ulises and myself agreed to. It's a nice to have, but unless we open the discussion to have this throw behavior as the default, the setting itself (default off) can come in as a minor. |
|
Ok, so how about this. To keep things moving, lets go back to not setting it by default and then we can land it either now if we get approvals, or we can release it as a minor soon after the main release. |
|
I updated the tracking todos to move this to the "future minor" |
lib/application.js
Outdated
| @@ -99,6 +99,7 @@ app.defaultConfiguration = function defaultConfiguration() { | |||
| this.set('query parser', 'simple') | |||
| this.set('subdomain offset', 2); | |||
| this.set('trust proxy', false); | |||
| this.set('strict status codes', true); | |||
There was a problem hiding this comment.
please update this back to false as default
thank you, by the way ❤️
|
Ok, so how about this. To keep things moving, lets go back to not setting it by default and then we can land it either now if we get approvals, or we can release it as a minor soon after the main release. |
aagamezl
force-pushed
the
aa/feat/strict-status-code
branch
from
18331b2 to
82d0a9c
Compare
History.md
Outdated
| * `res.status()` accepts only integers, and input must be greater than 99 and less than 1000 | ||
| * will throw a `RangeError: Invalid status code: ${code}. Status code must be greater than 99 and less than 1000.` for inputs outside this range | ||
| * will throw a `TypeError: Invalid status code: ${code}. Status code must be an integer.` for non integer inputs | ||
| * By default `res.status()` accepts only integers, and input must be greater than 99 and less than 900 |
|
@jonchurch That's my bad, I don't think I was clear, when I wrote:
I meant I'd prefer no configuration. Revisiting this a few months later I actually think this is still correct, and it should be loose by default, and anything else can be middleware. My hot take here is that we should actually be deprecating and removing configuration instead of adding more. Edit: I'm not going to block on configuration being added though, that's just my 2c on the overall subject. |
I probably agree, that said this PR is in line with historical design of the project. I am sorry for any churn I created by asking the setting to be on by default. I think we should work toward aligning on some of these "technical vision" ideas after we release v5 and work toward applying those ideals in v6 (where landing this as a minor could be a step toward that, or closed if we decide the goal is to remove configuration options). |
|
To be clear, what folks agreed to proceed with was the validation of the range of what node accepts -- there was discussion about limiting the range further, about whether the framework should be more or less opinionated, and so on, but the idea of a |
test: test for status in range with strictt status refactor: default strict status codes to true feat(status): set strict status codes to false docs: fix History message
aagamezl
force-pushed
the
aa/feat/strict-status-code
branch
from
82d0a9c to
0dae369
Compare
| @@ -129,6 +129,20 @@ describe('res', function () { | |||
| .expect(500, /Invalid status code/, done); | |||
| }); | |||
|
|
|||
| it('should raise error for status code above 599', function (done) { | |||
There was a problem hiding this comment.
Can you also add the test case for status code less than 100, if in future this PR is accepted
According to issue, a new configuration variable is added and the status code behavior is adjusted.