expressjs / express

Fast, unopinionated, minimalist web framework for node.
https://expressjs.com
MIT License
65.71k stars 16.32k forks source link

consider removing app.configure() #936

Closed tj closed 10 years ago

tj commented 12 years ago

pros:

cons:

todo:

tj commented 12 years ago

will add a deprecation warning for the first few minor releases

defunctzombie commented 12 years ago

What should be done instead? Just if statements? to check for the environment you want? It would also be nice to be able to activate the express js 'production' environment when not using the 'production' for NODE_ENV. Just thinking about people who might have different types of environments but still want to turn on cachine and other production internals of express.

tj commented 12 years ago

you can already with app.set('env', 'whatever'), though env variables are nice for env-related things haha so i definitely think that's the best practice

tj commented 12 years ago

looks uglier, but less obscure, im still iffy about it since it looks so cluttered, but you're usually making subtle tweaks to middleware for different envs


var app = express();

switch (app.settings.env) {
  case 'production':
  case 'stage':
    app.set('stuff', 'here');
    break;
  default:
    app.set('stuff', 'for dev here');
}

or for ex:


var app = express();
var env = app.settings.env;

app.set('foo', 'bar');
app.set('bar', 'baz');
app.set('baz', 'raz');

if ('production' == env) {
  app.use(express.logger())
} else {
  app.use(express.logger('dev'))
}

if ('production' == env) {
  app.use(express.static('/uploads'))
} else {
  app.use(express.static('/tmp'))
}

app.use(app.routes);
app.listen(3000);
defunctzombie commented 12 years ago

The if case seems nice and at least gets the point across that you can setup the middleware however you want.

Are there any internals that are activated when the env is 'production' like view caching etc? That is the only thing I had in mind really when thinking about activating the production settings from a different environment. If everything can be activated manually that is better :)

tj commented 12 years ago

yeah, a common pattern for myself is duplicating the middleware setup per env and tweaking, which is fine but it only works well for smaller apps. nothing is hard-coded to an env, they're all settings, people should do this in their own apps too it's dirty to check the env specifically

pesho commented 12 years ago

+1 for this change, the pros outweigh the cons. App.configure() by itself is not flexible enough, I've often had to add ifs.

tj commented 12 years ago

maybe what we should do is leave it in for 3x but undocument it

defunctzombie commented 12 years ago

I think removing it would be better. If you just undocument it, people that have it in their codebase won't remove it (why would they if it works) and then it will just break when you actually do remove it. Since updating code to not use it is backwards compatible with the 2.x line I think removing it would be fine. If you think about how a typical deployment could play out, a developer can easily update the code for 3.x (remove configure) and still be working fine on 2.x. To me, that means it should be pretty safe to remove.

tj commented 12 years ago

it's safe to remove, but it removing so much might scare people away haha, we could add it back in the compatibility portion so people can migrate easier

matthewmueller commented 12 years ago

I definitely think that app.configure() outweighs the proposed alternatives here. As for the pros of removing it:

• less misleading since people often think it's required I feel like if this is a problem than it's a documentation issue.

•  middleware config in separate function calls is difficult to manage This indicates a problem with the organization of our application.

• it's unclear that the functions are executed immediately This seems valid, maybe the function implies it will be "configured" and then applied when the application is run. I think this is something you learn once and you won't have any problems with in the future.

I think app.configure() is a pretty elegant solution to adding middleware and setting up your application.

tj commented 12 years ago

one other pro for app.configure() is that you could do say:

app.configure(function(done){
  fetch stuff from db for
  boot here and done()
});

app.configure(function(done){
  fetch moar stuff from db for
  boot here and done()
});

app.listen()

to defer listen(), however that's really not anything but a little flow control lib, but I definitely prefer to keep things flat like that

defunctzombie commented 12 years ago

@visionmedia I did not know you could defer the listen like that. Given that use case, I could see some potentials for leaving it in and maybe getting rid of the "production" or "development" configuration types and let users use if statements within the single configure block or how many ever blocks they need.

tj commented 12 years ago

it's tough because there's no 100% awesome solution to configuration that I've found at least. sometimes it's great to just use json, sometimes js, sometimes a mixture, sometimes you need external config anyway for other associated projects blah blah. There's a plugin that lets you defer like that but if configure() stays I'll probably add it to 3x.

ritch commented 12 years ago

What if we just moved it out into a plugin that monkey-patches express with configure. Remove the internal dependencies and then the documentation can be clear both in the migration guide and in this new plugin...

matthewmueller commented 12 years ago

What about allowing a JSON object to be passed through app.configure()?

This would allow you to do something like:

app.configure('production', require('./configuration'));

Then you could additionally define settings using JSON exports.

tj commented 12 years ago

json isn't really flexible enough

matthewmueller commented 12 years ago

Ohh oops. I forgot to mention allow configure to take both functions and objects. If it's a function, behave the same as before, if it's an object, update the settings.

ghost commented 12 years ago

I say get rid of configure. It's only 7 lines of code (in express/lib/http.js). All it does is obscure things.

tj commented 12 years ago

leaving it for now, people are already sketched about the removal of partial/layouts, so let's make things a little easier :)

defunctzombie commented 11 years ago

It has already been removed from the app template. Maybe we can add a deprecation notice now in 3.x and purge in 4.x thoughts @jonathanong @visionmedia

jonathanong commented 11 years ago

yeah deprecation notice would be nice. maybe a little annoying though since people call app.configure() multiple times so we gotta make sure to only warn once.

@visionmedia want to give us admin rights to the expressjs.com repo? to move express stuff to an org?

jonathanong commented 11 years ago

i actually want to remove the app template as well. it's pretty opinionated and there are plenty of other generators out there now that do it better IMO. i'd rather have express refer users to those.

defunctzombie commented 11 years ago

You know we could provide a nice deprecation notice with the if statement that would do the exact same thing that should make people happy. On Nov 19, 2013 7:23 PM, "Jonathan Ong" notifications@github.com wrote:

i actually want to remove the app template as well. it's pretty opinionated and there are plenty of other generators out there now that do it better IMO. i'd rather have express refer users to those.

— Reply to this email directly or view it on GitHubhttps://github.com/visionmedia/express/issues/936#issuecomment-28853035 .

jonathanong commented 11 years ago

yup, and/or a link to this issue

hacksparrow commented 11 years ago

Hey Jon, which generators would you recommend?

On Wed, Nov 20, 2013 at 6:55 AM, Jonathan Ong notifications@github.comwrote:

yup, and/or a link to this issue

— Reply to this email directly or view it on GitHubhttps://github.com/visionmedia/express/issues/936#issuecomment-28856231 .

jonathanong commented 11 years ago

don't know them by name since i don't use them. i know there are some yeoman related ones. i know strong loop is also making one.

some other options are to just have an "ideal" example, or move the generator to a separate repo and make it more community-oriented by giving more people push rights.

defunctzombie commented 11 years ago

+1 for removing generator and just leaving the examples. Express is meant to be easy to get started with by hand without a generator :)

I thought about the app.configure() stuff and have come to the following conclusion; remove it in 4.0 no deprecation warning now. Many people will simply not see the warning since they may not track 3.x upgrades all the time and we can better document it on a 3.x -> 4.x migration wiki.

jonathanong commented 11 years ago

here's a crazy generator i just ran across on reddit: https://github.com/paypal/kraken-js but it's super opinionated and it's made my paypal, so...

hacksparrow commented 11 years ago

Interesting.

The app structure generated by most generators can be justified depending on their requirements, but it is important to let developers (esp. beginners) know that architecting an app is distinct from Express, the framework. They can either have a one-page app, or more complex setups like

It might be a good idea to keep the built-in generator; if anything, make it simpler, if possible. Yeoman / Grunt should not become a dependency. Let's keep Express self-contained and very beginner-friendly.

On Wed, Nov 20, 2013 at 11:18 AM, Jonathan Ong notifications@github.comwrote:

here's a crazy generator i just ran across on reddit: https://github.com/paypal/kraken-js but it's super opinionated and it's made my paypal, so...

— Reply to this email directly or view it on GitHubhttps://github.com/visionmedia/express/issues/936#issuecomment-28865356 .

jonathanong commented 11 years ago

not against having a generator, just against having it in this repo. like i said above, i prefer if it were in its own separate repo and more community-driven. i don't even mind if we include it with express as a dependency. i feel bad for PRs for the generator since it's very useful to some people, but people just keep asking for features and we're just like "oh god, no."

the generator isn't something we maintainers use since we think express is simple enough to write out by hand. the generator should be built and maintained by those who don't agree or are new and know the learning curves.

hacksparrow commented 11 years ago

And yes, app.configure should go. I had to lookup the docs to remember what it is actually.

On Wed, Nov 20, 2013 at 12:06 PM, Hage Yaapa captain@hacksparrow.comwrote:

Interesting.

The app structure generated by most generators can be justified depending on their requirements, but it is important to let developers (esp. beginners) know that architecting an app is distinct from Express, the framework. They can either have a one-page app, or more complex setups like

  • Ghost, Sails, Kraken, etc.

It might be a good idea to keep the built-in generator; if anything, make it simpler, if possible. Yeoman / Grunt should not become a dependency. Let's keep Express self-contained and very beginner-friendly.

On Wed, Nov 20, 2013 at 11:18 AM, Jonathan Ong notifications@github.comwrote:

here's a crazy generator i just ran across on reddit: https://github.com/paypal/kraken-js but it's super opinionated and it's made my paypal, so...

— Reply to this email directly or view it on GitHubhttps://github.com/visionmedia/express/issues/936#issuecomment-28865356 .

hacksparrow commented 11 years ago

I like that approach. The generator is a separate entity, so makes sense for it to be on a separate repo, at the same time it should be an integral part Express. Probably we could make something bigger and more useful out of it. Here is a suggestion:

Create a system for developers to contribute their generators, which in turn would be used by the express command to generate the app defined in the generators.

$ express generate basic

$ express generate ghost

$ express generate kraken

$ express generate sails

etc.

All the frameworks built on Express are basically opinionated Express apps, the express command should be able to generate them if someone wants to use that particular architecture.

On Wed, Nov 20, 2013 at 12:15 PM, Jonathan Ong notifications@github.comwrote:

not against having a generator, just against having it in this repo. like i said above, i prefer if it were in its own separate repo and more community-driven. i don't even mind if we include it with express as a dependency. i feel bad for PRs for the generator since it's very useful to some people, but people just keep asking for features and we're just like "oh god, no."

the generator isn't something we maintainers use since we think express is simple enough to write out by hand. the generator should be built and maintained by those who don't agree or are new and know the learning curves.

— Reply to this email directly or view it on GitHubhttps://github.com/visionmedia/express/issues/936#issuecomment-28867142 .

aheckmann commented 11 years ago

Create a system for developers to contribute their generators, which in turn would be used by the express command to generate the app defined in the generators.

-1 For having that in express. +1 for a completely separate module.

That's all off-topic though :)

tj commented 11 years ago

-1 for generator stuff from me, i dont really get why yeoman is a thing, you could just clone a repo haha

jonathanong commented 11 years ago

@visionmedia -1 for moving the generator stuff or -1 for all the additional features?

defunctzombie commented 10 years ago

Done via ac2cbef8

ZainShaikh commented 10 years ago

Could someone please update the docs as well?

defunctzombie commented 10 years ago

@ZainShaikh those are the 3.x docs

ZainShaikh commented 10 years ago

@defunctzombie Oops, didn't notice. Thanks.