Deprecate non integer status codes in v4

[jonchurch]

This relates to #4212 and actually does two things. First, it uses res.status everywhere that the this.statusCode = N was previously used in res. That shouldn't be a breaking change. It allows us to check the status codes in order to deprecate. The changes related to using res.status internally are copied from #4212.

Wanted to open this as an option for inclusion in v4 to warn folks about breaking changes coming in v5 for their app.

The goal here is to only print a deprecation message for behavior which will throw under v5 but does not currently throw under v4. Specifically I want to avoid users seeing a deprecation about something throwing soon, and then immediately after also seeing the Node.js throw error for bad statuses. I can already see the Github issues if that were the case.

There are two dep messages, in order to hopefully help folks understand specifically what is happening and how the behavior will change.

The two cases where a dep message will print:

• String values in range of what Node.js accepts as valid, i.e. '200' and '304.5'. All strings throw under v5
• Non integer values in range of what Node.js accepts as valid i.e. 200.5.

[dougwilson]

First, it uses res.status everywhere that the this.statusCode = N was previously used in res. That shouldn't be a breaking change

It is unfortunate, but I tried to do this kind of change a while back and it had to be reverted as it was a breaking change. This is because folks will override res.status to do things and they didn't expect them to be called in places where they were not before.

Wanted to open this as an option for inclusion in v4 to warn folks about breaking changes coming in v5 for their app.

This is a good idea!

The goal here is to only print a deprecation message for behavior which will throw under v5 but does not currently throw under v4. Specifically I want to avoid users seeing a deprecation about something throwing soon, and then immediately after also seeing the Node.js throw error for bad statuses. I can already see the Github issues if that were the case.

Yep, I agree on this point for sure 👍 If we already don't accept a value, then there isn't anything to actually deprecate, since it already doesn't work, if I'm understanding correctly.

[dougwilson]

The deprecation should be making use of the same deprecation system currently as use, namely leveraging the depd pacakge. This has many, many benefits over console.log, which is why it is in use.

[jonchurch]

Oh woops, this was just a copy paste error! I pasted over a version I was testing where I replaced deprecate with a log.

[dougwilson]

The deprecation message should really be worded like all the rest, namely:

1. Not refer to a future version of this module (i.e. do not say "Express vFuture will ....")
2. Inform the user what they are supposed to do instead; in other words what is their course of actions (i.e. "string "200" is deprecation, pass as a number 200").

If it were me, as an example, I would word this specific message as deprecate("res.status(" + JSON.stringify(code) + "): use res.status(" + code + ") instead"). This would, given the call res.status("200") print the message express deprecated res.status("200"): use res.status(200) instead app.js:4:54

[jonchurch]

Updated the dep message

[jonchurch]

@dougwilson Yes you understand the aim of when to print the message, only on behavior that goes from non-throwing at all to throwing in v5.

That's a shame about res.status, would have been such an easy way to print this dep for all statuses 😭.

Is there anything you suggest? Reverting those changes so only res.status includes this check is the simplest, but I really want to try and give a dep notice to anyone who will see throws in v5. I could make a checkForDeprecatedStatus function and utilize it everywhere a status code is set in response. What would you think about that?

[dougwilson]

What would you think about that?

That is fine, as the depd will still make the stack trace just fine as long as the helper is in the same file.

[dougwilson]

Hey @jonchurch before I try to get this merged tomorrow, just wanted to comment here: is this all ready from your pov and nothing outstanding you know of? Just checking in before landing it is all 👍

[jonchurch]

@dougwilson Yes this is ready from my pov

[dougwilson]

Awesome, thank you @jonchurch ! I will add a test for the one missing line I see is being reported to fix the status failure on the PR 👍

[dougwilson]

P.S. as an updated, a few days ago I pushed up (as a separate commit) some generalized tests around the full functionality of res.status, which would then by it's nature cover the new branches and lines added in this PR (and thus fulfilling the test coverage). Of course, it turns out that the io.js fork like has different behavior in the edge cases that we are deprecating compared to all the node.js lines... I was thinking of how to best address it, as just not including those particular tests would still have some tests, except it wouldn't fully cover the new branches added here, haha.

I don't think there is any reasonable way to "feature sniff" the difference after spending a lot of time looking into it -- but if someone knows, please correct me! I'm just going to add some code to skip those tests on the io.js builds, as it seems the only reasonable way. The fact that there is even this weird discrepancy kind of leads credence to the throwing of this behavior in Express directly in 5.0 as this is pushing towards, as the specific handling of these edge cases Express can at least make consistent when the Express API is being used.