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

catch async errors doesn't call next when there is no error, so it's useless #25

Open
kapouer opened this issue Oct 17, 2024 · 6 comments

Comments

@kapouer
Copy link

kapouer commented Oct 17, 2024

Hi,

const express = require('ultimate-express');
const app = express();

app.use(async (req, res) => {
   console.log("mw", req.path);
});
app.get('/a', (req, res) => {
   res.sendStatus(200);
});

app.listen(8080, () => { });

then this hangs curl http://localhost:8080/a.

It would have been nicer if async middlewares had their next() method automatically called.

@cesco69
Copy link
Contributor

cesco69 commented Oct 17, 2024

Also vanilla Express stuck into the middleware. You must call next. This is the normal behaviour of Express

@kapouer
Copy link
Author

kapouer commented Oct 17, 2024

I know that, but this behavior renders the "catch async errors" useful only for terminal routes.

@kapouer
Copy link
Author

kapouer commented Oct 17, 2024

From https://github.com/davidbanham/express-async-errors?tab=readme-ov-file#a-notice-about-calling-next
we can see that the intended use allows automatic call of next()

app.use(async (req, res) => {
  const user = await User.findByToken(req.get('authorization'));

  if (!user) throw Error("access denied");
});

@cesco69

This comment was marked as outdated.

@dimdenGD
Copy link
Owner

I can see in documentation that it's supposed to do that, but for some reason it doesn't work for me (hangs) even on normal Express

const express = require("express");
require('express-async-errors');

const app = express();
app.set('env', 'production');

app.use(async (req, res) => {
    console.log("mw", req.path);
});

app.get('/a', (req, res) => {
    res.sendStatus(200);
});

app.listen(8080);

@nigrosimone
Copy link
Contributor

nigrosimone commented Oct 20, 2024

@dimdenGD

express-async-errors monkey patch router and router/layer
see https://github.com/davidbanham/express-async-errors/blob/master/index.js#L1 and https://github.com/davidbanham/express-async-errors/blob/master/index.js#L29

only can work with express! The only way is rewrite it for ultimate-express, eg. the first two lines

from

const Layer = require('express/lib/router/layer');
const Router = require('express/lib/router');

to

const Layer = require('ultimate-express/src/router/layer');
const Router = require('ultimate-express/src/router');

This isn't an issue for ultimate-express but a bad design of express-async-errors

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

No branches or pull requests

4 participants