Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

root app errors are ignored by mounted apps #1522

Closed
nickjackson opened this Issue · 17 comments

5 participants

@nickjackson

If you try and next(new Error) on a root app, the error will be ignored by any mounted apps.

I don't think this is an issue as such and maybe a lame thing to add. The only reason for asking is because I am trying to mount a component app which handles errors for the whole app.

At the moment, I am binding to the 'mount' event and adding the error middleware to the root app in the callback. This works fine, however it does feels a little clunky.

@jonathanong

i'm guessing you want to do something like:

var root = express()
var errorHandler = express()

root.get('stuff', ...)
root.post('stuff', ...)

root.use(errorHandler)

your error handler needs to have 4 arguments, with the first beign err, so you'll have to do something like this:

root.use(function (err, req, res, next) {
  errorHandler(req, res) 
})

i don't know how you'll pass the error, though. personally, i don't think this feature will ever hit express because the use case is so little. reopen if you have a suggestion.

@jonathanong jonathanong closed this
@ianstormtaylor

+1 to this

it seems leaky that everything passes through except for when error-arity middleware are concerned. i was also just trying to have a mounted subapp with all of our error pages self-contained. instead i'm also resorting to using on('mount', ... to attach directly to the parent

@jonathanong

it doesn't make any philosophical sense to me. if you have an error in your subapp, it should be handled by any error handlers in your subapp, otherwise by the parent app. if you have an error in your parent app, it should be handled by the parent app's error handler, not any mounted subapp, unless a subapp is explicitly mounted as an error handler.

why don't you just do something like this?

app.use(function (err, req, res, next) {
  console.log(err.stack) // handle error
  errorHandlerApp(req, res)
})

plus, that just sounds a maintenance burden to implement.

@ianstormtaylor

that seems equally janky to me tho

i think it starts to make more sense when you think of them more as just collections of middleware that get pushed onto the stack, in the order they are added, instead of as all being equal and separate children themselves. the apps already cascade like normal middleware on regular requests

the subapp i'm envisioning is solely in charge of rendering error pages, returning straight JSON or nicely formatted HTML depending on how the request is formed. the actual parent app responsibilities (eg. logging the errors) would still be handled in the parent app because thats in its own scope, but errors pages or api responses shouldnt need to be

does that make more sense?

@defunctzombie
Collaborator

This sounds more complex than it needs to be to me. Why not just use routes? What does a subapp get you? If you actually do have separate top level "apps" i.e. /calendar, /movies, or whatever then I personally fall under the opinion that you should either use routers or just use a proxy-like server to serve to all the apps you instantiated. Maybe what you are really looking for is a proxy that understands it only proxies to express apps and has nothing to do with settings or whatnot. An app is just a container for some global settings which are exposed to the routes it uses. Using another app is very weird to me since they are not a parent->child relationship by the nature of there typically being a single "app".

tl;dr; use routes!

@jonathanong

i think it starts to make more sense when you think of them more as just collections of middleware that get pushed onto the stack, in the order they are added, instead of as all being equal and separate children themselves.

yup, this is how apps should be organized. however, mounting subapps is the wrong way to do it because apps are self-contained with their own error handling and settings. this is why i want to push towards mounting routers instead of apps in #1843. if you want to use() a stack as middleware, just use that middleware instead of all the baggage it comes with.

@ianstormtaylor

hmm, kinda makes sense, but i don't quite agree. i actually like that settings are self-contained. that means an app can decide on it's own views directory, engine, etc. and the parent shouldn't need to know about those things. but pure error middleware seems different than subapp-specific settings, in that every other instance of middleware doesn't have the limitation, so the pure stack of routing functions doesnt seem right

i dont think i like the router solution because i'm using sub apps to contain app-specific settings in the first place

what i'm pretty sure i want is an easy way to push a group with its own internal settings onto the stack of middleware..

@tj
tj commented

I'm half and half too, I agree with @ianstormtaylor with a lot of the settings, even though that's partly a "fail" of express coupling views to the app settings in the first place haha. The router use-case makes sense for a lot of situations but not all of them

@defunctzombie
Collaborator

Can you elaborate on what an app specific setting would be? To me, if all your stuff is running in the same process, it is the same "app". If you have app specific view dirs, etc then you have several apps and now you just have a "routing" issue which is where I talked about the "super app" idea which isn't an app but really just a top level router that delegates the request to the specific app. This "super app" would have no settings and just delegate.

@jonathanong

i think we're talking about general app architecture now. there are valid use-cases for mounting sub apps with their own self-contained settings and such. the routing thing is to make it easier for people to mount sub apps vs. mounting routes depending on their use-case. right now, it's hard for people to mount routes without the baggage of apps.

however, for this specific use-case, it doesn't make sense to me why a sub app's error handling middleware should handle its parent's errors. every app should only handle its own errors.

@tj
tj commented

yea the apps do have some baggage, but it would be nicer IMO to fix that baggage and have one way of passing stuff. I think as far as error handling goes it's fine in the "root" app, as middleware and the children delegate up, maybe not quite ideal as far as tempting goes but not too brutal

@jonathanong

it would be nicer IMO to fix that baggage

sounds like work :D i like the routing solution because it makes things simpler and doesn't require much effort, but i agree that it's only a short-term solution.

and i should have added to that last comment, "handle its own errors or delegated from its subapps"

@ianstormtaylor

hmm yeah, honestly if views were handled differently that would eliminate most of my use of app settings, and then i'd be down with the idea of just pure routers i think. i would def want errors to cascade into the groups though (but with thin routers at that point that shouldnt be a problem). still agree with tj tho that one way to do it (apps, not routers) seems like the nicer one

@jonathanong

if views were handled differently

you should open an issue/PR with suggestions before express 4! something like co-views is the way to go IMO.

@tj
tj commented

derp

@defunctzombie
Collaborator
@tj
tj commented

it's my conflict resolution strategy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.