4.17.0 breaks contract of response.status

[emiln]

From the release notes of 4.17.0:

Improve error message for null/undefined to res.status

This implies that it's a matter of how the error is communicated, but the 4.17.0 release actually makes it an error, which it wasn't before.

If any application calls response.status(null) or response.status(undefined), this will now break their app, where before it would work just fine.

Particularly, some apps I've worked on set response.status(undefined) early in the middleware chain so they're able to decide, late in the middleware chain, whether any decision was made by middlewares about which status code to respond.

All these apps are broken with 4.17.0

[dougwilson]

Thanks you for the feedback. We definitely do not want to break existing apps. The change was available to test for a least 5 months prior to the release (#3778). We try to be careful but there are many unexpected use cases for Express.

As a side note of the report, can you share some thoughts on how we can improve sharing the changes ahead of time for compatibility testing? Should the window for releases be larger than 5 months lead time?

[dougwilson]

As one more ask (sorry!) would you be able to share an example of what you want to keep working so we can add it to our test suite to ensure continued functioning of whatever it is these apps are doing? A PR to add the test (don't worry if it fails CI) or just a simple app shared here would be awesome to make sure that whatever change gets landed will address your use case 👍

[emiln]

Thanks for the fast reply.

I honestly don't think there's much hope in reaching people in my position. We use express as dependency wrapped inside a bunch of conventions about how we want to do microservices. So it's part of a much larger meta package. This meta package depends on a non-locked version of express, so we've now been getting the 4.17.0 for any new projects we create and any old project that updates its dependencies.

We could certainly have found the issues by testing ahead of time, but there are so many dependencies to keep track of, and we just honestly do not keep track of what's to come in any of them.

I don't think you could have prevented this from happening through any reasonable communication means. We will generally only be looking at new dependency versions when we need to, not as a forethought kind of thing.

I'm a little torn about whether to fix our meta package's use of express or whether to ask you to revert anything. There's an easy fix of using response.statusCode = undefined instead of response.status(undefined), so we're in no way stuck.

If you want to pin down the old behavior of accepting undefined (something that was never explicitly mentioned - neither as allowed nor disallowed), I'll draft up a MR for a new test. I'll take a stab at that today or tomorrow. It's nearing end of workday in Denmark.

[wesleytodd]

Thanks for the feedback @emiln. I would guess that if you have this issue it is also effecting many others who have yet to discover it or have not opened issues. So it is reasonable to consider backing that out and waiting for 5.0 to make the error change. Just my 2c.

[emiln]

The current tests for .status are written with supertest to test a full request->response flow. In this case it would be perfectly fair to have e.g. .send throw a sensible error if the .statusCode property isn't set at that point, since Node.js doesn't produce a response at all in this case.

Would you accept a couple of new tests that pin down that behavior? You should be allowed to call .status with null and undefined, but you shouldn't be allowed to call .send unless you've set a sensible status code?

[dougwilson]

Sorry I have been at work today and have not responded. I think even that could still have the same surprise for someone else that this had for you. Based on this issue, I think there is nothing we can do in the 4.x series around this. As an example, someone could be using on-headers module or similar to set a status code, which would happen after res.send is entered and there isn't a good point at which to detect such a thing.

I agree with @wesleytodd here that we should just revert the behavior in the 4.x series and not touch anything around this.

@emiln I think you for your feedback. Your answer is perfectly acceptable, as there is no way to catch everyone, so thanks for reflecting on it :) If you're interested in contributing tests to help lock in the behavior in 4.x that would be great 👍 . We should lock it to whatever the pre-4.17.0 behavior is and not introduce any new behavior around here due to the likely fragility.

[emiln]

Excellent. I'll write us a couple of tests to pin the old behavior tomorrow at work.

[kamronbatman]

From an outside perspective/opinion, response.status(null) and response.status(undefined) doesn't seem like a normal input or even an abnormal input that should have an expected or desired result. To me it seems like the previous behavior was a happenstance that was abused more than an actual deliberate feature.

[wesleytodd]

@kamronbatman I think we all agree on that. Still, the fact that it has been abused means that we cannot release a semver minor/patch which makes this change. It must go with the next semver major. Luckily that should be soon!

[kamronbatman]

I was about to start crying because we have been waiting for Express 5 forever. Of course, it takes as long as it takes, and it is a major overhaul, so I understand and appreciate waiting until it is ready.

Is it possible to cherry pick and revert this for a quick 4.17.1 (or 4.18.0?)

[dougwilson]

I was about to start crying because we have been waiting for Express 5 forever. Of course, it takes as long as it takes, and it is a major overhaul, so I understand and appreciate waiting until it is ready.

We had a meeting at the beginning of this year and knocked off all the not-already-completed items from the 5.x backlog and then the plan was to roll the next Express minor (4.17 in this case) into the 5.x release and spin it out as a 5.0. So 5.0 is the next release, before a 4.18 :)

Is it possible to cherry pick and revert this for a quick 4.17.1 (or 4.18.0?)

That is the plan I think from the above: revert this change for a 4.17.1.

[LinusU]

We could potentially log a deprecation message if res.status is called with null or undefined 🤔

[wesleytodd]

We could potentially log a deprecation message if res.status is called with null or undefined 🤔

If we are going to remove it in 5 that seems like a good idea.

[dougwilson]

So I plan to revert the commit and release today for those who are encountering the issue. Typically a deprecation would appear like a new feature, in a minor release. Of course the minor release here broke folks and it seems like reverting it back is going to be the solution. I would propose rather than trying to add in more things into a patch, just revert and release as 4.17.1, then we can open a PR for the deprecating behavior to land in 4.18. WDYT?