consider removing app.configure() #936
Comments
ghost
assigned 
|
will add a deprecation warning for the first few minor releases |
|
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. |
|
you can already with |
|
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); |
|
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 :) |
|
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 |
|
+1 for this change, the pros outweigh the cons. App.configure() by itself is not flexible enough, I've often had to add ifs. |
|
maybe what we should do is leave it in for 3x but undocument it |
|
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. |
|
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 |
|
I definitely think that • less misleading since people often think it's required • middleware config in separate function calls is difficult to manage • it's unclear that the functions are executed immediately I think |
|
one other pro for 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 |
|
@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. |
|
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. |
|
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... |
|
What about allowing a JSON object to be passed through This would allow you to do something like: Then you could additionally define settings using JSON exports. |
|
json isn't really flexible enough |
|
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. |
|
I say get rid of configure. It's only 7 lines of code (in express/lib/http.js). All it does is obscure things. |
|
leaving it for now, people are already sketched about the removal of partial/layouts, so let's make things a little easier :) |
|
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 |
|
yeah deprecation notice would be nice. maybe a little annoying though since people call @visionmedia want to give us admin rights to the expressjs.com repo? to move express stuff to an org? |
|
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. |
|
You know we could provide a nice deprecation notice with the if statement
|
|
yup, and/or a link to this issue |
|
Hey Jon, which generators would you recommend? On Wed, Nov 20, 2013 at 6:55 AM, Jonathan Ong notifications@github.comwrote:
|
|
don't know them by name since i don't use them. i know there are some 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. |
|
+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. |
|
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... |
|
Interesting. The app structure generated by most generators can be justified depending
It might be a good idea to keep the built-in generator; if anything, make On Wed, Nov 20, 2013 at 11:18 AM, 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. |
|
And yes, app.configure should go. I had to lookup the docs to remember what On Wed, Nov 20, 2013 at 12:06 PM, Hage Yaapa captain@hacksparrow.comwrote:
|
|
I like that approach. The generator is a separate entity, so makes sense Create a system for developers to contribute their generators, which in etc. All the frameworks built on Express are basically opinionated Express apps, On Wed, Nov 20, 2013 at 12:15 PM, Jonathan Ong notifications@github.comwrote:
|
-1 For having that in express. +1 for a completely separate module. That's all off-topic though :) |
|
-1 for generator stuff from me, i dont really get why yeoman is a thing, you could just clone a repo haha |
|
@visionmedia -1 for moving the generator stuff or -1 for all the additional features? |
|
Done via ac2cbef |
|
Could someone please update the docs as well? |
|
@ZainShaikh those are the 3.x docs |
|
@defunctzombie Oops, didn't notice. Thanks. |
This is deprecated, and removed in 4.0 (expressjs/express#936). Use an explicit environment check instead.
pros:
app.settings.envcons:
todo:
The text was updated successfully, but these errors were encountered: