express icon indicating copy to clipboard operation
express copied to clipboard

Settings functionality moved out of core

Open wesleytodd opened this issue 7 years ago • 10 comments

This is based on what was discussed at the last TC meeting for a good "first step" in abstracting out parts of core than can be shared. The changes here are rather far reaching, but they basically mean the following:

app.set()
app.get()
app.enable()
app.disable()
app.enabled()
app.disabled()

Become:

app.settings.set()
app.settings.get()
app.settings.enable()
app.settings.disable()
app.settings.enabled()
app.settings.disabled()

I think there are a bunch of points for discussion here:

  1. I opted to move it all under .settings, mainly to resolves the weird behavior from app.get being multi purpose. But it could also be done so that the methods remain the same, either via mixing them in, or explicitly calling into .settings as getter methods.
  2. If we are already breaking the api, should we also add other changes, like removing the enabled/disabled shorthands to minimize the api surface.
  3. Do people like the "setters" type functionality? It is used to keep the features like setting etag but it actually setting etag fn. Not sure, but it seems to me we could get rid of the whole thing but just using etag directly.

EDIT: standardjs compliance pending :) **Dont feel like doing all that busy work tonight if the whole PR might get shut down...

wesleytodd avatar Feb 21 '17 03:02 wesleytodd

Is there something I can do on this PR to move it forward?

wesleytodd avatar Jun 11 '18 21:06 wesleytodd

Love it. There hasn't been any objections or anything, and finally gets rid of those annoying overloads like .get(setting). I plan to land this right in the next 5.x release coming here (which will also include the new Promise-supporting router).

dougwilson avatar Aug 02 '18 22:08 dougwilson

Awesome!! I will go through and do a rebase/cleanup pass asap.

wesleytodd avatar Aug 02 '18 22:08 wesleytodd

Sweet, that will help a lot 👍 I had noticed two things, mainly, from looking through it at a high level: (1) cap the dependency to ~ instead of ^ (or pin) and (2) I was curious to see if there would be some way to back port this into 4.x such that there could be a deprecation cycle on the removed APIs (but I don't know if that's possible -- and it's not a requirement to land something breaking into 5.x, of course).

dougwilson avatar Aug 02 '18 22:08 dougwilson

Yeah I can cap that for sure. For the back porting, I think we can do it, just need to call into the new stuff and log the deprecation in the old. I will take a look at that as well.

wesleytodd avatar Aug 02 '18 22:08 wesleytodd

Nice.

dougwilson avatar Aug 02 '18 22:08 dougwilson

Ok, so the changes in 4.x are so different that it will actually be better for me to make two separate PR's. Other than the examples, basically every line changed is a conflict. I will open up a new PR with the 4.x compatible changes and then see what is required to the 5.0 support. But unfortunately it has eaten up all of my time today to be sure that is the best way to move forward. Maybe next weekend :)

wesleytodd avatar Aug 06 '18 00:08 wesleytodd

Ok, #3714 is open. It ports all these changes to 4.0 and provides deprecation warnings. Once we figure out the points I brought up over there I will finalize the changes in this PR for the 5.0 branch.

wesleytodd avatar Aug 11 '18 21:08 wesleytodd

What is the status of this PR?

abrahamcuenca avatar Jan 21 '20 06:01 abrahamcuenca

@wesleytodd did this die off? Looking at https://github.com/expressjs/express/pull/2237 it seems sensitive to ask about 5.0 considering it is locked. Considering that PR was created on 14 Jul 2014 I think it is a fair question (although I do know these topics are sensitive to maintainers, but 7 years is a long time).

thernstig avatar Jan 22 '21 14:01 thernstig