express icon indicating copy to clipboard operation
express copied to clipboard

Move settings methods to app.settings and deprecate old versions

Open wesleytodd opened this issue 5 years ago • 3 comments

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.

wesleytodd avatar Aug 11 '18 21:08 wesleytodd

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 avatar Aug 11 '18 21:08 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.

dougwilson avatar Aug 11 '18 21:08 dougwilson

Release discussion: https://github.com/expressjs/discussions/issues/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 avatar Aug 11 '18 22:08 wesleytodd