agrueneberg / Corser

CORS middleware for Node.js
MIT License
91 stars 11 forks source link

Origins callback doesn't follow Node error convention? #8

Closed aseemk closed 11 years ago

aseemk commented 11 years ago

Hey there,

Great work on Corser. I just had one piece of feedback from my initial looking into it: is there a particular reason the origins callback doesn't follow the typical Node (err, matches) convention? Instead, its callback just takes (matches).

This may not be a big deal, but if you're deriving the answer async'ly and you get an error, there's no way to propagate this error here, e.g. to error logging middleware later in the pipeline. It also doesn't play nicely with async control flow tools and libraries.

Not a huge deal, just sharing this feedback. Great work again and thanks!

agrueneberg commented 11 years ago

Hi Aseem,

thanks for pointing out this mistake. When I wrote Corser I didn't anticipate errors (:+1:), but @Nomon's contribution of dynamic origin checking changed that. I missed my chance to figure out how Connect does error catching at that point, and this how it happened. I propose to follow the error handling convention found in Connect, as implemented in branch v2-error-handling. Unfortunately, this change is not backward compatible, so it will be part of Corser 2.0 coming soon.

-ag