carbon-io / carbond

MIT License
2 stars 5 forks source link

Using .sync on middleware throws an error #217

Open tfogo opened 7 years ago

tfogo commented 7 years ago

Middleware and error handling middleware are checked for the number of required arguments using Function.length here: Service.js#L1160

However some middleware might use the arguments object to manage their arguments in which case Function.length would be 0.

Specifically, fibrous does this in its synchronize function. This is an issue if you need to run .sync on middleware.

tfogo commented 7 years ago

Looked into this a bit more. Express actually checks the arity of middleware itself using Function.length. Express will silently pass over middleware that doesn't have the correct arity.

Now, sync middleware will still work in express because express only passes over middleware if fn.length > 3. So sync functions with an arity of 0 get called as expected.

However, express will pass over error handling middleware if fn.length !== 4. This means that error handling middleware will not work if called using .sync.

So removing the arity check from _initializeMiddleware will fix the issue with sync middleware. However, removing the arity check from _initializeErrorHandlingMiddleware is not sufficient to fix the issue with using sync on error handling middleware.

tfogo commented 7 years ago

So what's the play here? I'd say remove the arity checks from _initializeMiddleware.

_initializeErrorHandlingMiddleware is more difficult. We may need to hack fibrous for it to work. 😕 ... but it may not be that big a deal. Maybe make a note in the docs that if you need to use error handling middleware using sync you should wrap it:

(err, req, res, next) => { yourMiddleware.sync(err, req, res, next) }