vkarpov15 / awaitjs-express Goto Github PK

getAsync use IRouterMatcher that doesn't allow return promise from handler:

router.getAsync('/user', async (req, res) => {
   
})

Promise returned in function argument where a void return was expected  @typescript-eslint/no-misused-promises

the same problem as in #25

The only thing holding me up from using this module is the Typescript Declarations.

Running tsc gives the following errors:
node_modules/@awaitjs/express/index.d.ts(4,13): error TS2314: Generic type 'IRouter<T>' requires 1 type argument(s). node_modules/@awaitjs/express/index.d.ts(5,16): error TS2314: Generic type 'IRouter<T>' requires 1 type argument(s). node_modules/@awaitjs/express/index.d.ts(6,13): error TS2314: Generic type 'IRouter<T>' requires 1 type argument(s). node_modules/@awaitjs/express/index.d.ts(7,15): error TS2314: Generic type 'IRouter<T>' requires 1 type argument(s). node_modules/@awaitjs/express/index.d.ts(8,14): error TS2314: Generic type 'IRouter<T>' requires 1 type argument(s). node_modules/@awaitjs/express/index.d.ts(9,13): error TS2314: Generic type 'IRouter<T>' requires 1 type argument(s). node_modules/@awaitjs/express/index.d.ts(10,14): error TS2314: Generic type 'IRouter<T>' requires 1 type argument(s).

Versions I'm using:
@awaitjs/[email protected]
[email protected]
@types/[email protected]

My code is blow:

  const r = Router()
  // model/
  r.getAsync<any, any, any, any>('/', async (req, res, next) => {
    const { query } = req
    let { where, pageSize = Infinity, page = 1 } = query
    page = Math.max(0, page - 1)

    // throw error
    const { rows, count } = await modelClass.findAndCountAll({
      limit: pageSize,
      offset: isNaN(pageSize * page) ? 0 : pageSize * page,
      where: {
        ...where
      }
    })

    console.log(rows, count)

    onSuccess({ list: rows, page: { pageSize, page, total: count } }, res, req)
  })
  .use((err, req, res, next) => {
      // not work
      res.send(String(err))
  })

I thought res.headersSent is true in after async processor, But Readme's example which throws error in sync mode

While using the package I noticed some weird behavior in my express app.
After digging deeper into the matter I found two inconsistencies.

1. The package ignores whether the passed next() callback was executed or not in normal non-async middleware. Accordingly to express documentation if next() hasn't been called, such requests should be left hanging:

If the current middleware function does not end the request-response cycle, it must call next() to pass control to the next middleware function. Otherwise, the request will be left hanging.

2. As a consequence of the first issue, old-style non-promise async middlewares are working incorrectly.
   For example, a middleware with such body will pass the execution to the next middlaware, without waiting for it's execution to complete:

function (req,res,next) {
  setTimeout(() => {
    req.testMessage += '3';
    next();
  }, 500);
}

It makes the package usable only when each middleware returns a Promise. Usual callback-based middlewares are not handled correctly.

First off, thanks for creating another awesome lib @vkarpov15 :)

I am seeing some sort of racecondition but have troubles spotting it reliably why its happening (or if this has nothing to do with awaitjs/express).

Following is an abstraction of what I am doing.
Generally what I want is to have multiple different files, each with a Router from @awaitjs/express that are then put together like the following;

//tags.ts
export const tagsRouter = Router({ mergeParams: true });
tagsRouter.getAsync('/*', async (req, res) => {
	console.log('Tag routes..')
	await new Promise<void>((resolve) => {
		setTimeout(resolve, 5000)
	})
	console.log('Tags finished..')
	next()
})
tagsRouter.getAsync('/*', async (req, res) => {
	const data = await something()
	console.log('Do Tag..')
	res.json(data)
})
// more routes...

//models.ts
export const modelsRouter = Router({ mergeParams: true });
modelsRouter.getAsync('/*', async (req, res) => {
	console.log('Model routes..')
	await new Promise<void>((resolve) => {
		setTimeout(resolve, 5000)
	})
	console.log('Models finished..')
	next()
})
// more routes...

//main.ts
import {tagsRouter} from 'tags'
import {modelsRouter} from 'models'
import {ensureAccess} from '../access';
export const mainRouter = Router({ mergeParams: true });

mainRouter.use(':userId/*', ensureAccess); //some access control which makes a async call to the database
mainRouter.use(':userId/tags', tagsRouter);
mainRouter.use(':userId/models', modelsRouter); 

When running this from time to time I get the outputs in a wrong order

Tag routes..
Do Tag..
Tags Finished..

instead of

Tag routes..
Tags Finished..
Do Tag..

Am I messing something up in my setup? Am I misunderstanding this library?

Wanted to know if decorateApp can/should be used on a Router instance as well?

// In `app.js`
const express = require('express');
const { decorateApp } = require('@awaitjs/express');
const app = decorateApp(express());


// in some route file
const express = require('express');
const { decorateApp } = require('@awaitjs/express');
const router = decorateApp(express.Router());

router.getAsync('oops', async (req, res, next) => {
    throw new Error('Oops!');
});

There doesn't seem to be a way to end request processing with a wrapped app. Given next() is not needed anymore and res.end() doesn't seem to have an effect, it looks like it's not possible to implement things like a router that stops a non authenticated request when this middleware is used. Please advice if this is not correct / document a mechanism similar to not calling next() or calling res.end() that stops processing the request. :)

Express documentation shows an example,
https://expressjs.com/en/guide/writing-middleware.html

module.exports = function (options) {
  return function (req, res, next) {
    // Implement the middleware function based on the options object
    next()
  }
}
The middleware can now be used as shown below.
var mw = require('./my-middleware.js')
app.use(mw({ option1: '1', option2: '2' }))

When I tried this with postAsync, the compiler threw an error, saying that a promise was found where a function was expected; it worked if I modified the code and invocation
router.postAsync('/', async (req, res, next) => { await myAsyncMiddleware(req, res, next, myparameter) }, myOperation );
Can the express js recommended model be supported by awaitjs?
It's a lot cleaner to write
router.postAsync('/', myAsyncMiddleware(myparameter), myOperation);
than the longer model above.

A few things are wrong with the wrap definition.

• The isParam: boolean should be optional (e.g. isParam?: boolean)
• The Function type in param fn: Function is essentially useless and causes a compiler error

The defintion should look something like:

declare function wrap(fn: (...args: Parameters<RequestHandler>) => PromiseLike<void>, isParam?: boolean): Function

I use nodemon to monitor for changes to my files as follows:

./node_modules/.bin/nodemon -r esm ./src/index.js

However after using this plugin in my code it appears the app doesn't close down properly and I get an EADDRINUSE error. Saving the code again and nodemon restarts and all works fine.

Every second save it works!

Sadly, I get an error when I use it in combination with the express-validation package from https://github.com/AndrewKeig/express-validation

Error:

TypeError: Cannot read property 'then' of undefined
    at wrappedMiddleware (C:\Users\Georgios\Documents\GitHub\node_modules\@awaitjs\express\index.js:100:23)
    at Layer.handle [as handle_request] (C:\Users\Georgios\Documents\GitHub\node_modules\express\lib\router\layer.js:95:5)
    at next (C:\Users\Georgios\Documents\GitHub\node_modules\express\lib\router\route.js:137:13)

[app|router].route() returns a non-async Router instance. meaning to use it with asynchronous functions, the following syntax is required:

// In some route file
const router = express.Router();
addAsync(router.route("/foo"))
.getAsync(...)
.postAsync(...);

Preferred syntax:

const router = addAsync(express.Router());
router.routeAsync("/foo")
.getAsync(...)
.postAsync(...);

When a promise is used, what is the preferred method of making sure the error in the promise is passed to next as you would with a non-async route handler?

In the example below should I:

1. not use catch
2. call next(err) (as shown below)
3. or throw err instead of calling next(err)?

// in some route file
const express = require('express');
const { decorateApp } = require('@awaitjs/express');
const router = decorateApp(express.Router());
const request = require('superagent');

router.getAsync('/', async (req, res, next) => {
    const thing = await request
        .get(`https://example.com/api/things/1`)
        .then(apiRes => apiRes.body)
        .catch(err => {
            next(err);
        });

    res.status(apiRes.statusCode);
    res.send(thing);
});

It would be nice to handle errors in async middleware that are mounted on a route as well. Currently the following middleware error would not be handled by the error middleware

const express = require('express');
const { decorateApp } = require('@awaitjs/express');
const app = decorateApp(express());

async function middleware(req, res, next) {
  throw new Error('From Middleware');
}

async function handler(req, res, next) {
  res.send('OK');
}

app.getAsync('/test', middleware, handler);

app.use(function(error, req, res, next) {
  res.send(error.message);
});

app.listen(3000);

It looks like currently only the last argument to [method]Async is wrapped if it's a function. Would it be easy enough to do the same for all arguments?

app.useAsync(async(req: Request): Promise<void> => { }) throws an error on version 0.9.0 with:

Expected at least 2 arguments, but got 1.

I believe this is just a typings issue because the definition for useAsync is trying to piggy back off of getAsync, postAsync, etc.

It would be nice to patch express.Router() on addAsync() so that all async methods are automatically available on routers as well.

I currently do this in my app:

const { addAsync } = require('@awaitjs/express');

const originalRouter = express.Router;

express.Router = function() {
  return addAsync(originalRouter.apply(this, arguments));
};

Express allows to pass a body parser as the second argument like this:

router.postAsync(
  '/log',
  express.raw(),
  async (req, res) => {...}
);

This actually works as expected, but TypeScript reports an error.