hoangvvo / next-connect

The TypeScript-ready, minimal router and middleware layer for Next.js, Micro, Vercel, or Node.js http/http2
https://www.npmjs.com/package/next-connect
MIT License
1.64k stars 65 forks source link

fixes #35

Closed throwarray closed 4 years ago

throwarray commented 4 years ago

fixes #30

throwarray commented 4 years ago

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

hoangvvo commented 4 years ago

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

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
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
hoangvvo commented 4 years ago

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 commented 4 years ago

Thanks @throwarray, close in favor of #38

throwarray commented 4 years ago

@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 commented 4 years ago
handler.get(async function () {
    await new Promise(function (resolve, reject) {
        setTimeout(function () { reject(new Error('error')) }, 100)
    })
})
hoangvvo commented 4 years ago

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