[@type/express-serve-static-core] Allow express RequestHandler to return Response

[xfournet]

In v4 we used to write handler with a compact syntax like this

app.get("/response", (_, res) => res.json('ok'));

This was possible because;

• from javascript perspective, express don't care about the returned value of the handler, so returning something doesn't matter
• from typescript perspective, due to the specific handling of void returning function, it's possible to return anything in a void function.

However, as noted in #70563 using void | Promise<void> cannot accept anything, including Response or Promise<Response>

The PR change the return type of RequestHandler to permit to return Response in addition to void (and the same for async/Promise)

---

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change.
• Follow the advice from the readme.
• Avoid common mistakes.
• Run pnpm test <package to test>.

[typescript-bot]

@xfournet Thank you for submitting this PR!

This is a live comment that I will keep updated.

1 package in this PR

• express-serve-static-core — on npm, on unpkg

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

• ❌ No merge conflicts
• ✅ Continuous integration tests have passed
• 🕐 Most recent commit is approved by a DT maintainer

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 18 days.

---

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 70696,
  "author": "xfournet",
  "headCommitOid": "f0980de79d0d937d26065f24a1c36510553b595c",
  "mergeBaseOid": "08f56a2bd0d71d12f468b93862aa555c7ec683bb",
  "lastPushDate": "2024-09-26T23:27:27.000Z",
  "lastActivityDate": "2025-01-28T00:47:05.000Z",
  "hasMergeConflict": true,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "express-serve-static-core",
      "kind": "edit",
      "files": [
        {
          "path": "types/express-serve-static-core/express-serve-static-core-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/express-serve-static-core/index.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "borisyankov",
        "micksatana",
        "JoseLion",
        "dwrss",
        "andoshin11"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "stale",
      "reviewer": "jakebailey",
      "date": "2024-10-29T19:21:51.000Z",
      "abbrOid": "b3402ab"
    },
    {
      "type": "stale",
      "reviewer": "RobinTail",
      "date": "2024-10-29T11:19:31.000Z",
      "abbrOid": "b3402ab"
    }
  ],
  "mainBotCommentID": 2378114426,
  "ciResult": "pass"
}

[typescript-bot]

🔔 @borisyankov @micksatana @JoseLion @dwrss @andoshin11 — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

[typescript-bot]

@xfournet The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds that are failing do not end up on the list of PRs for the DT maintainers to review.

[typescript-bot]

🔔 @xfournet — you're the only owner, but it would still be good if you find someone to review this PR in the next few days, otherwise a maintainer will look at it. (And if you do find someone, maybe even recruit them to be a second owner to make future changes easier...)

[jakebailey]

At this point, why even bother typing anything for the result? Why not just write unknown and let it be whatever?

Theoretically this is fine, I suppose, but just kinda weird.

@RobinTail

[RobinTail]

from javascript perspective, express don't care about the returned value of the handler, so returning something doesn't matter

if it does not matter, not taken into account and not processed then why are you trying to return something?
Moreover, the suggestion to return something meaningful would lead to confusion, implying that it would affect something, lead to a certain consequence, but it's not.
I see no reason/grounds for such a change, @xfournet

CC @jakebailey

[RobinTail]

compact syntax like this

if the compactness is your aim, you can still do it this way, @xfournet :

app.get("/response", (_, res) => void res.json('ok'));

But the strive for compactness does not justify incorrectness.

[jakebailey]

Or:

app.get("/response", (_, res) => { res.json('ok') });

[RobinTail]

Yes, but some code formatters, like Prettier, would make that statement multiline, so it might not be so desirably compact.
So, I kinda understand the concern, but its essence does not seem to me DT related.

[typescript-bot]

Re-ping @borisyankov, @micksatana, @JoseLion, @dwrss, @andoshin11:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

[typescript-bot]

@xfournet One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

[xfournet]

Thanks all for your feedbacks, i run out of time and i'm a bit late to answer, sorry for that

@RobinTail

if it does not matter, not taken into account and not processed then why are you trying to return something?

my point is about backward compatibility with existing codebase for which Typescript is now failing while the code is still executing correctly from a javascript point of view. From the javascript perspective Express 5 is properly testing if the returned value is a promise, and only process the value in this case.
I simplified a bit my example, but like @Mauro-Domingues i see a lot a control-flow based on returning the response in our codebase.
The fact is that typescript void return is unrestrictive and this has lead some codebase to adopt such patterns given the v4 type definition. Should we really break them ? I known it's a major version, but no deprecation path like for other Express changes could be a bit abrupt and prevent some migration to Express 5.

Moreover, the suggestion to return something meaningful would lead to confusion, implying that it would affect something, lead to a certain consequence, but it's not.

Yes you're right i'm changing it to unknown | Promise<unknown>

@jakebailey

At this point, why even bother typing anything for the result? Why not just write unknown and let it be whatever?

Yes, i updated the PR with unknown | Promise<unknown> to be less confusing. Note that we can't just use unknown in case people (like me) using eslint @typescript-eslint/no-misused-promises rule (error: Promise returned in function argument where a void return was expected)

[typescript-bot]

@jakebailey Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

[jakebailey]

I am not sure why the tests aren't failing with this. This is equivalent to writing "unknown" because the union is reduced. I don't think this is a good type to use.

[xfournet]

see my previous comment

Note that we can't just use unknown in case people (like me) using eslint @typescript-eslint/no-misused-promises rule (error: Promise returned in function argument where a void return was expected)

From a strictly TypeScript perspective, you're correct; we could even have kept void as in v4 since void doesn’t limit the return type, allowing Promise to be returned with void or unknown alone. However, this approach can be cumbersome for those using @typescript-eslint/no-misused-promises, which requires the return type to explicitly reference Promise.

[jakebailey]

I understand, but that sure seems like a linter bug. unknown | number is unknown. The types are eaten.

I'm not sure how they can detect this unless they are somehow reading out the AST for the originally declared type, which will only ever work for hand-written code. If you wrote this code in TS today, it's absolutely free to not reprint this type as a union.

[jakebailey]

See also from ts-eslint, https://typescript-eslint.io/rules/no-redundant-type-constituents/

[xfournet]

I'm not sure how they can detect this unless they are somehow reading out the AST for the originally declared type, which will only ever work for hand-written code. If you wrote this code in TS today, it's absolutely free to not reprint this type as a union.

you're right, i double checked my test, and i should have done something wrong previously. Returning a promise on a function declared has unknown is not reported from @typescript-eslint/no-misused-promises.
I'm changing the PR

[typescript-bot]

@xfournet One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

[typescript-bot]

@jakebailey, @RobinTail Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

Old

[jakebailey]

Code wise, the PR is fine, but I think I'd like to see some sort of consensus that this should change.

[RobinTail]

I think we're facing a conceptual problem here: should DT describe the types strictly to ensure constraints that help to avoid mistakes, OR should those types be loose enough to fit:

• imperfections,
• desire for neatness,
• unwillingness to add curly braces,
• particular ESLint rules.

I've been personally leaning to the first opinion, but I understand that DT used by millions of people with different opinions on that matter. So, I'm going to refrain and rely on opinion of DT maintainers, @jakebailey

[andrewbranch]

DT types are supposed to describe the public contract of the package, without being overly burdensome. For example, I recently discouraged a contributor from typing each color component of an RGB input option as a union of all integers between 0 and 255—that’s correctness that imposes too great a burden on API consumers.

I personally don’t love this change, as I think void has some marginal documentation value. But this is a decision we’d like the types maintainers to make.

[Mauro-Domingues]

Thanks all for your feedbacks, i run out of time and i'm a bit late to answer, sorry for that

@RobinTail

if it does not matter, not taken into account and not processed then why are you trying to return something?

my point is about backward compatibility with existing codebase for which Typescript is now failing while the code is still executing correctly from a javascript point of view. From the javascript perspective Express 5 is properly testing if the returned value is a promise, and only process the value in this case. I simplified a bit my example, but like @Mauro-Domingues i see a lot a control-flow based on returning the response in our codebase. The fact is that typescript void return is unrestrictive and this has lead some codebase to adopt such patterns given the v4 type definition. Should we really break them ? I known it's a major version, but no deprecation path like for other Express changes could be a bit abrupt and prevent some migration to Express 5.

Moreover, the suggestion to return something meaningful would lead to confusion, implying that it would affect something, lead to a certain consequence, but it's not.

Yes you're right i'm changing it to unknown | Promise<unknown>

@jakebailey

At this point, why even bother typing anything for the result? Why not just write unknown and let it be whatever?

Yes, i updated the PR with unknown | Promise<unknown> to be less confusing. Note that we can't just use unknown in case people (like me) using eslint @typescript-eslint/no-misused-promises rule (error: Promise returned in function argument where a void return was expected)

I have the habit of having explicit return on my functions because it is a good practice and helps other developers to understand the code. In fact, removing the "return" and leaving it as Promise<void> solves the problem, but some factors can influence this change such as documentation generators and human errors because Promise<void> is not restrictive. But I am consented to the change.

[jakebailey]

Given the lack of any further requests for this (here, in issues, or discussions), I feel like this can be closed.

[typescript-bot]

@xfournet Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day!

[KalenAnson]

I just got bit with this issue (re: expressjs/express#5987).

I would argue that allowing express to return from a route hander only improves the readability of code and promotes secure operation of the handler.

Most handlers are not quite so simple as this:

app.get("/response", (_, res) => res.json('ok'));

Often there is loads of validation (often offloaded to middleware) and business logic in the route handler.

Letting the route handler call return res.send() helps developers express in simple terms that the route handler is now done™. I have used this paradigm since express 1.0 to avoid shooting myself in the foot. Pulling this very intuitive backstop makes it very easy to have the handler do bad things non-intuitively.

Love to see this decision reconsidered.