Comments (12)
This complaint seems odd to me. The cause of the error in that example was failure to return
a promise:
app.use((ctx, next) => {
console.log('MW2');
next(); // <-- we lose error handling here because promise is not returned
});
However, the exact same situation occurs in when using async
function if you don't await
it.
app.use(async (ctx, next) => {
console.log('MW2');
next(); // <-- error handling still lost, didn't use `await`
});
Forcing async functions doesn't solve this issue (without something like a lint rule requiring every async
function to include an await
, which STILL wouldn't be bulletproof). With promises you just have to either await
or return
them if you want error handling to work. Trying to catch every developer error seems like fighting the tide.
from discussions.
I think what @bttmly meant was that it is the developer's job to learn how to use promises and async functions. I agree with you @DesignByOnyx however you can't pad mistakes so they function properly. The only way to learn is through trial and error :).
Perhaps a link in the docs to promise docs and async / await is the way to go.
from discussions.
I also wanted to include other findings to contribute with this discussion. The following mix of sync and async middleware results in a soon-to-be-fatal UnhandledPromiseRejectionWarning
:
const Koa = require('koa');
const app = new Koa();
app.use(async ({response}, next) => {
try {
return await next();
} catch (ex) {
console.log('MY ERROR', ex.stack);
response.body = ex.stack;
}
});
app.use(async (ctx, next) => {
console.log('MW1');
await next();
});
// The next two are the problem - koa should warn IMO
app.use((ctx, next) => {
console.log('MW2');
next();
});
app.use((ctx, next) => {
console.log('About to throw');
throw new Error('Please');
});
NOTE: Changing the last two middleware to async/await or returning a promise fixes the issue.
from discussions.
I personally think this is to be expected. If a user returns a Promise s/he should expect surrounding try-catch to be out of context. In order words, should one choose to do manual promise handling, one should also be expected to error handle accordingly (e.g. add .catch
method in your first example).
I would personally not like Koa not accepting common promise patterns (currently running native async-await comes with a decent performance hit, and should node 8 not upgrade to v8 5.7 this "problem" will persist through node 8 LTS as well).
If anything, might I suggest a state or setting, e.g. app.asyncFunc = true
which makes sure AsyncFunction
is passed to .use
? This could be done without having to change koa-compose.
edit opted for app.forceAsync
in PR.
from discussions.
I think I diverged form you a little in my use of of the term "common". In my previous examples I was using "common" to refer to sync (single tick) functions - which is what breaks. Expanding on my 3rd example (with the 4 middleware), the problem is fixed by making the "common" middleware (MW2
) return a promise. To me, this makes the middleware "async"- hence the suggestion to "force" everything to be async. So yes, I agree with you on the use of "common" (not async/await) functions as long as they return a promise (or some other then-able mechanism?).
Here is my 3rd example reworked to be completely "async" using a mix of common functions and async/await. This works and the comments describe "why".
const Koa = require('koa');
const app = new Koa();
// this MUST be async/await if you want to avoid using `next().catch()`
app.use(async ({response}, next) => {
try {
return await next();
} catch (ex) {
console.log('MY ERROR', ex.stack);
response.body = ex.stack;
}
});
// "common" functions should return a promise
app.use((ctx, next) => {
console.log('MW1');
return new Promise((resolve, reject) => {
next().then(resolve, reject);
});
});
// OR "common" functions can return `next()`
// Docs should mention that 'next' always returns a promise
app.use((ctx, next) => {
console.log('MW2');
return next();
});
// This sync middleware only works because it's last in the stack
// Ideally, koa would throw saying that
// 1. You should use an 'AsyncFunction'
// 2. OR, the middleware should return a promise
app.use((ctx, next) => {
console.log('About to throw');
throw new Error('Please');
});
So to clarify, my suggestion is to:
- require all middleware to be async/await (like you did in your PR)
- OR, require all middleware to return a promise (this is still missing)
from discussions.
async function is just a common function return a promise(but it won't throw error synchronouslly). so we can treat them as the same. don't await next() in async function means breaking the promise chain will cause unexpected results, this is the basic rule we must follow in every middleware and we can't help more.
but IMO we should handle common function's error in koa-compose, because some other modules like koa-router also use koa-compose and support common functions. koa-compose composed all the middlewares(async functions, common functions) in to one function return a promise, we should keep errors can be catched in promise way.
from discussions.
koa-compose handles synchronous errors just fine already. They are caught and turned into a rejected promise. See: https://github.com/koajs/compose/blob/master/index.js#L43-L48
This issue is unrelated to that, it's purely about the need to await
or return
a promise.
from discussions.
@nickb1080 - thanks for your comments. The solution doesn't require any complex linting or "catching every developer error" or anything like that. It just needs to make sure (as you pointed out) that middleware either 1) implements AsyncFunction
, or 2) returns a promise. This is very easy to detect from code and providing a warning to users would save a lot of head scratching.
You point out a 3rd case where someone might use async
without using await
- and this falls under the "poorly written code" category IMO - I have no desire to try and detect this scenario as the developer is writing code that doesn't make sense.
from discussions.
I would put both missing an await
and missing a return
under "poorly written code". They have the same semantic meaning in their respective cases.
An opt-in option to force AyncFunction
seems fine, but if its not the default setting – and to make it so would be a definite breaking change – it doesn't seem like it'll fix a whole lot. Once the confused developer finds the docs for that feature they'll have identified exactly the problem (perhaps by landing on this very thread). There is also an issue there where you might not be able to use some third-party middleware that is written with regular ol' promises. Further, an extremely common case that async
/await
just doesn't support – parallel operations (think Promise.all
or Bluebird.props
) – would need to be wrapped in a pointless async
function to satisfy the AsyncFunction
check.
My view is async
/await
is syntactic sugar and the nature of the underlying promise system will leak though no matter what.
from discussions.
I would put both missing an await and missing a return under "poorly written code".
I guess I disagree completely here. Defining an async
function that never await
s anything is poorly written, anit-pattern'y code which doesn't make any logical sense to write. Not returning from a function is normal every day code - most connect-style middleware never returns at all. Needing to know that you should always return a promise is not obvious, at least not to me (I'm no amateur).
parallel operations (think Promise.all or Bluebird.props) – would need to be wrapped in a pointless async function to satisfy the AsyncFunction check.
I don't think all-or-nothing for AysncFunction is the solution, and I don't really agree with the ' opt-in option to force AyncFunction'. My intent was never to force AsyncFunction
, but for "force" (maybe by gently warning users) that middleware must either be AsyncFunction or return a Promise. Failure to do so breaks an app.
Go back and read my initial question and the very very simple code therein - that code is a broken app. I don't consider it "poorly written code" - it's very basic code written by an experienced developer. There is no indication why it's broken. 2 lines of code could warn developers of this very innocent and easy-to-make mistake.
For the record, I am by no means trying to tread into idiot checking every line of code a developer writes. I don't think this even gets close to that.
from discussions.
Would the error handling wiki be a good place to summarize this? I ran into this error and was scratching my head for a bit not knowing where to look at first (I had forgotten to return next()
in a sync middleware function I made, of course) and I was clicking around the main site and didn't notice anything that pointed me in the right direction. I'd like to put a note in there to save the next person a bit of time/searching
from discussions.
Sorry if this might be offtopic, buy migrating from koa 1.x, I have also run into the case where I would await next
(mistakenly omitted async function call, thinking of yield next
) and the app would silently stop passing to the next middleware - maybe this is also worth noting, especially if coming from koa 1.x
from discussions.
Related Issues (20)
- Improving the Koa ecosystem by moving all Koa modules to the koajs organization! HOT 62
- Immutable empty query obj HOT 15
- How to exclude api router? HOT 1
- Access to XMLHttpRequest at 'https://localhost:3443/images' from origin 'http://localhost:4200' has been blocked by CORS policy
- Destroy Mounts
- Conditional middleware
- Support for spa server?
- how to reg two part to the same file
- learning koa 2 and updating examples:https://github.com/aline2013/koa2-examples
- how to set interval within stream
- Hosting mp3 file
- Either 404 or wrong content HOT 1
- Error jump URL when ctx.path is directory
- koa-static: heavy caching issue HOT 2
- Why do i need koa-static HOT 1
- 404 not found for everything HOT 2
- koajs/conditional-get: uws.http failure
- Using both koa-compress and koa-static , extremely slow when visit static files from 127.0.0.1 or localhost (from public address is fast), why? HOT 2
- Brotli compress is slow HOT 2
- @fl0w becomes @miwnwski HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from discussions.