Move settings methods to app.settings and deprecate old versions

[wesleytodd]

Started as a 5.x PR, this is the deprecation version for the 4.x branch. See: #3218

The only thing I am worried about this this is that the app.settings object has changed. It used to be a plain object, and is now an instance of Settings. I think some purists would call that a breaking change. The problem is that there is no way to provide the new functionality without changing that object.

If we had a faster major release cadence, we could deprecate in 5.0 (breaking direct .settings access) and remove in 6.0. But that might be YEARS, and speeding up majors is a different topic, one which I think we should do. So IMO I think we have two options:

1. Live with this as a breaking change in an undocumented api and release the deprecation warnings on the documented stuff
2. Only do this in 5.0.

I am perfectly happy with either, but if I had a choice it would be 1 because it preps people who are using it for 5.0 and the churn on this api shouldn't be much and is worth it.

[dougwilson]

The only thing I am worried about this this is that the app.settings object has changed. It used to be a plain object, and is now an instance of Settings. I think some purists would call that a breaking change. The problem is that there is no way to provide the new functionality without changing that object.

Ah, never realized that. Yea, it may not be possible to land this in 4 then. But I am on my phone so need to actually look at the changes later to get a better feel.

and speeding up majors is a different topic, one which I think we should do.

I've had similar thoughts recently and I agree it is a different topic, so won't even share my thoughts on this inline to keep on topic. If you would like to open a new discussion in the discussion repo, feel welcome!

I am perfectly happy with either, but if I had a choice it would be 1 because it preps people who are using it for 5.0 and the churn on this api shouldn't be much and is worth it.

Yea, I'll take a look both at the implementation and as much existing code that is out there to see if we will definitely break something or not as an additional point of reference.

[dougwilson]

I believe the last few minors had changes that were super subtle breaking changes that ended up having quick .1s each time to revert them because the age of express meant there was just massive usage of those subtle things because even when we don't document it, it gets in blogs, books, and stackoverflow, making it... difficult, haha.

[wesleytodd]

Release discussion: expressjs/discussions#67

And yeah the support issues is a big deal, and honestly one of the things you have done a great job with in express over the years. If we don't think it can land in 4 then it is fine, I can just add deprecation warnings that say it will be removed in 5.0. Of if that other discussion bares fruit, we can move to deprecate in 5.0 and remove in 6 :)

[wesleytodd]

Ok, this one is old and I think some more tests were added which were not in there when this was originally written. I am pretty sure that what is broken here is related to other changes which landed after the separate package was made. I am going to need to revisit that package first and then decide what to do here but I don't have time for that today as I am writing this on an airplane.

[wesleytodd]

Yeah at some point I decided to archive that repo since it was not moving forward. The more I think about it the more I am sure we dont need another package for this, but the changes to get rid of the overloading of .set are IMO good changes, and ones which if we don't land in v5 mean we cannot make them for a while. The PR is at least conflict free now, so if we decide to pick this back up it should be relativly easy to fix the behaviors needed to pass the new tests, I am just not sure that is the best idea.

[wesleytodd]

After a discussion on the working session we decided to see if we can land this before the release. The push I just did is the rebase and getting the tests passing again in the process. I want to sleep on this change and see if I can improve but would love if folks had time to take a quick read as well in case there is something major blocking this.

Aside: the entire settings api needs a major overhaul, this is not attempting to do that. It is simply taking some work from 2017 and seeing if we can at least move the needle on fixing the overloaded .get, the weird indirection of .set, and the very bad arity based apis.

[bjohansebas]

Just a suggestion, we should have a few tests to ensure that the deprecated functions are working correctly, since now all the tests pass with the new approach.

[wesleytodd]

Just a suggestion, we should have a few tests to ensure that the deprecated functions are working correctly, since now all the tests pass with the new approach.

Agree completely! This is one of the things I was thinking about as I was updating this last night. Some of these methods have very unexpected behavior (at least to me) which has tests. I was pretty confident we would need to duplicate about half of the config.js test file to cover it, so I didn't want to do that last night, but I will start by fixing some of the other things, then I will make sure to add tests to cover this aspect.

[wesleytodd]

Attempting to get this updated I recall how much I hate this api. Not only do we have load bearing method overloading, arity checks and prototypical inheritance for setting lookup but also we directly expose all of this to end users in a way that means it is nearly impossible to introduce even small changes without them being breaking.

In my 2018 naivety I thought I could maybe fix some of this without it breaking things. I was wrong. The only ways we can make this non-breaking is:

1. if I add a net new api (not changing app.settings to be an instance of this class, maybe like app.settingsStore but would be open to better ideas here)
2. if we use a Proxy

EDIT: ok, I think I may be able to do some other horrible things with prototypical inheritance and hacking around the old api so that it maintains support for some of the access patterns but the new api wouldn't. Going to see if I can make that work and push a new commit (we can back that one out if we don't like it).

I am quite confident that Proxy would be a perf nightmare, and I am firmly on the fence on if a new api is a good idea.

I am going to continue noodling on this locally but wanted to post my fresh morning thoughts on this while I was thinking them. Maybe someone else has some good ideas to help here?

[wesleytodd]

Ok, this new commit is silly but it works. I added some tests and reverted some back to test the old behavior again. This is the important new bit:

https://github.com/expressjs/express/pull/3714/files#diff-17c1ca7f930cd47c534a89fbdfe848b46c6fe27f464f13afd5af6236b3d1dd25R66-R119

Instead of using a proxy, I kept the old pojo with prototypical inheritance (which I dont like but 🤷 maybe we can get rid of it for v6) and used a bit of reflection to expose the new api while only changing two things about settings:

1. It's prototype is now the classes' .settings property who's protoype is null
2. When assigning to a name from the new api, prints a deprecation warning (as I was writing this I noticed a case not covered in the old tests that needs one more change I will push next)

All of the old api's with assigning directly to app.settings, app.locals.settings, res.locals.settings work as expected and also update the new setting store correctly AFAICT so we get expected behavior even when using both api's.

Honestly reading this after writing it my conclusion is that this is making it significantly more complicated for a hopeful v6 future where it gets much more simple. I am not convinced this is a good trade, but this api is such a disaster that moving folks off of it may be worth it. Anyway, I need to go afk for a bit after this so feel free to tear this whole thing apart lol.

[wesleytodd]

@UlisesGascon I am thinking we need more time on this one. I was going over the tests and there are a bunch of cases which are possible for applications to use even if not documented in that way which I think we need to cover with tests before landing this. I can work on those, but from a priority and risk perspective I think it is too large of a change to land this rushed. It is more important that we go latest imo. Thoughts?

[bjohansebas]

btw, I would like to work on launching a new codemod for this new settings implementation so that the adoption of this change is as widespread as possible, which would require some time for work in it.

[UlisesGascon]

So... as discussed with @wesleytodd this PR won't be included in the 5.1.0 release and ideally we can include that in the next 5.x release but we should also create a small blog to explain this deprecation and how we handle deprecation in Express (in general).