deepkit-framework icon indicating copy to clipboard operation
deepkit-framework copied to clipboard

DX: Bug?: HTTP: better `next()` usage docs

Open alpharder opened this issue 1 year ago • 2 comments

It's currently stated at documentation:

All middleware needs to execute next() sooner or later. If a middleware does not execute next() withing a timeout, a warning is logged and the next middleware executed. To change the default of 4seconds to something else use timeout(milliseconds).

This middleware does what is said above:

import { HttpMiddleware, HttpRequest, HttpResponse } from '@deepkit/http';

export class AuthenticationMiddleware implements HttpMiddleware {
  constructor() {}

  async execute(
    request: HttpRequest,
    response: HttpResponse,
    next: (err?: unknown) => void,
  ) {
    response.statusCode = 403;
    response.end();
    next('qwe');
  }
}

Which results in a following error AND incorrect 404 response code:

2024-07-06T06:30:30.395Z [LOG] 127.0.0.1 - GET "/auth/whoami" 404 ""
102 | 
103 |         this._implicitHeader()
104 |       }
105 | 
106 |       if (!stream) {
107 |         return _end.call(this, chunk, encoding)
                          ^
error: write after end
 code: "ERR_STREAM_WRITE_AFTER_END"

      at new NodeError (node:stream:420:20)
      at _write (node:stream:2672:20)
      at node:stream:2852:76
      at end (/Users/alpharder/dev/kachalka/node_modules/compression/index.js:107:21)
      at handleHtmlResponse (/Users/alpharder/dev/kachalka/node_modules/@deepkit/http/dist/esm/src/http.js:720:32)
      at handle (/Users/alpharder/dev/kachalka/node_modules/@deepkit/http/dist/esm/src/http.js:3:16)
      at /Users/alpharder/dev/kachalka/node_modules/@deepkit/http/dist/esm/src/http.js:800:28
      at onResponse (/Users/alpharder/dev/kachalka/node_modules/@deepkit/http/dist/esm/src/http.js:797:16)

It's either should be fixed or it should be stated in the documentation that ending a response at middleware is either not allowed or doesn't require calling next().

Additionally, it's currently not documented what err argument of next() actually does (causes 404 response with empty body). This is also weird, but should at least be documented.

alpharder avatar Jul 06 '24 06:07 alpharder

the timeout was removed. middlewares were added to somewhat support some basic express middlewars like compress, but it was more a hack to get basics working and is not in line with the philosophie of deepkit and thus never fully added support for it, so we actually had the idea to remove it entirely and add some other way to support some useful middlewares. an alternative route could be to find all the rough edges and make it stable enough

marcj avatar Jul 06 '24 06:07 marcj

@marcj I'd say that listeners are much more powerful and polished.

We only need a DX-friendly way to attach listeners to only certain routes.

Does supporting express middlewares provide any value? Most of them are somewhat spaghetti unmaintained code.

IMO middlewares could be completely ditched

alpharder avatar Jul 06 '24 07:07 alpharder