Uh oh!
There was an error while loading. Please reload this page.
feat(response): new setting strict status codes - #5856
Conversation
Uh oh!
There was an error while loading. Please reload this page.
ctcpip
commented
Aug 23, 2024
I think we need to add a test that is outside the strict range -- I could not find any existing |
wesleytodd
left a comment
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?
61c44d7 to
f5a28b0Compareaagamezl
commented
Aug 23, 2024
@ctcpip I added the missing test |
aagamezl
commented
Aug 23, 2024
@wesleytodd maybe I misunderstood, could you confirm the default value of the setting? Default: true or |
wesleytodd
commented
Aug 23, 2024
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. |
f5a28b0 to
18331b2Compareaagamezl
commented
Aug 24, 2024
@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. |
wesleytodd
commented
Aug 28, 2024
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" |
| 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 ❤️
jonchurch
left a comment
There was a problem hiding this comment.
based on chats, we are requesting this is off by default ❤️
wesleytodd
commented
Aug 28, 2024
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. |
18331b2 to
82d0a9cCompare| * `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. |
wesleytodd
commented
Aug 30, 2024
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). |
ctcpip
commented
Aug 30, 2024
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
82d0a9c to
0dae369Compare| .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.