Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

add support for `next(status[, msg])` #703

Merged
merged 1 commit into from Jan 4, 2013

Conversation

Projects
None yet
2 participants
Contributor

defunctzombie commented Dec 7, 2012

When next is called with the first argument as a number, it will be
automatically converted to an Error object with the appropriate status
code.

The motivation behind this change is to make it easier for middleware to
indicate an error has occurred without needing to resort to custom error
classes.

Removes utils.error as it should no longer be used.

@defunctzombie defunctzombie add support for `next(status[, msg])`
When next is called with the first argument as a number, it will be
automatically converted to an Error object with the appropriate status
code.

The motivation behind this change is to make it easier for middleware to
indicate an error has occurred without needing to resort to custom error
classes.

Removes utils.error as it should no longer be used.
87ffafd
Contributor

defunctzombie commented Jan 4, 2013

Any more thoughts on this? Any downside? Biggest gain is easier error handling in routes and middleware.

Member

tj commented Jan 4, 2013

thought about it a little last night, I think maybe for now we add it back but leave it undocumented. I agree it's handy, still bugs me a bit api-wise just because next(500, 'whatever') doesn't have much to do with responding in general so it looks a little weird, and because people might start requesting things like next(500, 'foo', { extra error props }) and the list goes on but if we both mess around with it for a bit we can see how it goes, maybe I'll remember why I really disliked it before haha

@tj tj added a commit that referenced this pull request Jan 4, 2013

@tj tj Merge pull request #703 from shtylman/better-next
add support for `next(status[, msg])`
d309716

@tj tj merged commit d309716 into senchalabs:master Jan 4, 2013

1 check passed

default The Travis build passed
Details
Contributor

defunctzombie commented Jan 4, 2013

Sounds good to me :D

@defunctzombie defunctzombie deleted the defunctzombie:better-next branch Jan 4, 2013

Member

tj commented Jan 9, 2013

just realized we'll need this in the express router too

Contributor

defunctzombie commented Jan 9, 2013

Yea. Does it not just it it from connect?

On a side note. Having a use for the router might be nice. Helps when you
have separated routes and need some common thing to happen. I know you can
just get * ;)
On Jan 9, 2013 3:01 PM, "TJ Holowaychuk" notifications@github.com wrote:

just realized we'll need this in the express router too


Reply to this email directly or view it on GitHubhttps://github.com/senchalabs/connect/pull/703#issuecomment-12063700.

Member

tj commented Jan 9, 2013

it wouldn't pass the second value to connect, reverted it though hahaha #723 is a good enough reason to not have it IMO, that composition issue is definitely lame. However because of out-of-flow errors (#721 timeout is the only middleware doing this currently) we may have to change how the error handling works anyway

Contributor

defunctzombie commented Jan 9, 2013

Wait? You reverted the next change? Why? You don't have to lose error info.
Just make the error object with that info. I don't see how information is
lost.
On Jan 9, 2013 3:14 PM, "TJ Holowaychuk" notifications@github.com wrote:

it wouldn't pass the second value to connect, reverted it though hahaha
#723 #723 is a good enough
reason to not have it IMO, that composition issue is definitely lame.
However because of out-of-flow errors (#721https://github.com/senchalabs/connect/issues/721timeout is the only middleware doing this currently) we may have to change
how the error handling works anyway


Reply to this email directly or view it on GitHubhttps://github.com/senchalabs/connect/pull/703#issuecomment-12064354.

Contributor

defunctzombie commented Jan 9, 2013

Why is timeout a problem? When the request times out just prevent further
writing by the user.

Also, using domains I believe you can just capture any error thrown within
a route.
On Jan 9, 2013 3:14 PM, "TJ Holowaychuk" notifications@github.com wrote:

it wouldn't pass the second value to connect, reverted it though hahaha
#723 #723 is a good enough
reason to not have it IMO, that composition issue is definitely lame.
However because of out-of-flow errors (#721https://github.com/senchalabs/connect/issues/721timeout is the only middleware doing this currently) we may have to change
how the error handling works anyway


Reply to this email directly or view it on GitHubhttps://github.com/senchalabs/connect/pull/703#issuecomment-12064354.

Member

tj commented Jan 9, 2013

yeah we could maybe use domains, I still feel like they're a terrible thing but I guess node is a terrible thing when it comes to error handling in general. The problem is that for composing middleware they all have to be aware of this number -> error object conversion otherwise all the if (err) checks end up just passing along the number only and lose the message anyway

Contributor

defunctzombie commented Jan 9, 2013

I think if you look at the patch it converts that number to an error object
and uses that. That was my intention. The number should not be passed along
only the error object.
On Jan 9, 2013 3:43 PM, "TJ Holowaychuk" notifications@github.com wrote:

yeah we could maybe use domains, I still feel like they're a terrible
thing but I guess node is a terrible thing when it comes to error handling
in general. The problem is that for composing middleware they all have to
be aware of this number -> error object conversion otherwise all the if
(err) checks end up just passing along the number only and lose the
message anyway


Reply to this email directly or view it on GitHubhttps://github.com/senchalabs/connect/pull/703#issuecomment-12065601.

@defunctzombie defunctzombie restored the defunctzombie:better-next branch Jan 9, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment