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

DX: Bug?: HTTP: better next() usage docs #591

Open
alpharder opened this issue Jul 6, 2024 · 2 comments
Open

DX: Bug?: HTTP: better next() usage docs #591

alpharder opened this issue Jul 6, 2024 · 2 comments

Comments

@alpharder
Copy link
Contributor

alpharder commented Jul 6, 2024

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.

@marcj
Copy link
Member

marcj commented Jul 6, 2024

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

@alpharder
Copy link
Contributor Author

@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

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

2 participants