Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #35

Closed
wants to merge 3 commits into from
Closed

fixes #35

wants to merge 3 commits into from

Conversation

throwarray
Copy link

@throwarray throwarray commented Mar 17, 2020

fixes #30

@throwarray
Copy link
Author

Also, I'm curious what influenced the decision to remove (err,req,res,next) styled middleware. Perhaps it could be added again?

@hoangvvo
Copy link
Owner

Thanks @throwarray, I will review this PR soon. About the removal of (err, req, res, next):

  • This allows continuation after the error is throw (If needed).

Previous

app.use(thisWork)
app.use(thisThrow) // throw here
app.use(thisWork1) // always skip
app.use(thisWork2) // always skip
app.use(anerrormiddleware) // handle error
// it's over, no way back

Now

app.use(thisWork)
app.use(thisThrow) // throw here
// simply calling .onError here and if next is called...
app.use(thisWork1) // may continue
app.use(thisWork2) // may continue
  • It is awkwark always having to remember to put the error middleware at the end. If one forget:
app.use(thisWork) 
app.use(anerrormiddleware) // skip because there is no error
app.use(thisThrow) // throw here
app.use(thisWork1) // skip. uh oh where is the error middleware
app.use(thisWork2) // skip. oh no
  • The only way this can determine if a middleware is an error middleware is by looking at its number of argument (4 of them). This is unreliable. What if I do not need (eslint will complain if next is never used) or forget next argument?

@hoangvvo
Copy link
Owner

I think one thing I miss is the convenience of using multiple error middleware.

app.use(errorLogger)
app.use(anotherErrorLoggerBcWhyNot)
app.use(errorResponse)

Perhaps a future version will accept an array of middleware for onError

@hoangvvo
Copy link
Owner

Thanks @throwarray, close in favor of #38

@hoangvvo hoangvvo closed this Mar 25, 2020
@throwarray
Copy link
Author

@hoangvvo it does seem better to have it just be the apply method.
You should consider rewriting the 'next' function however because at the moment you have dangling promise errors. There's no catch on the next();. Or is that intentional?

@throwarray
Copy link
Author

handler.get(async function () {
    await new Promise(function (resolve, reject) {
        setTimeout(function () { reject(new Error('error')) }, 100)
    })
})

@hoangvvo
Copy link
Owner

@throwarray This is definitely a bug. I just reproduce it locally. I will try to get a fix in

@throwarray throwarray deleted the patch-1 branch May 27, 2021 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"API resolved without sending a response" appearing on every route despite the API routes working fine.
2 participants