Skip to content

Make it clear what arguments req.accepts* functions accept - #7452

Draft
krzysdz wants to merge 8 commits into
expressjs:masterfrom
krzysdz:acceptsX-tests-and-docs
Draft

krzysdz wants to merge 8 commits into
expressjs:masterfrom
krzysdz:acceptsX-tests-and-docs

Conversation

@krzysdz

@krzysdz krzysdz commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor
What pushed me to look into this (skip this if you value your time)

Background

#6088 changed the wording of req.acceptsCharsets() docs and introduced a mistake which went unnoticed until #7451, where it was reported as a bug in code. Interestingly, 9 days later #6936 fixed the same mistake in docs for req.accepts() and in #6936 (comment) @bjohansebas pointed out that Express had removed support for comma delimited string before 4.0 release. Historically it looks more or less like this:

  1. (2014) Express 4 switches to jshhtp/accepts, which does not accept a single comma delimited string, but leaves it in the docs.
  2. (2024-10-27) fix: enhance req.acceptsCharsets method #6088 is opened and mirrors the wrong example from req.accepts() to req.acceptsCharsets(). The PR appears to be at least AI-assisted, if not completely AI-generated, but this does not explain why nobody noticed this during review. It is possible that the text wasn't based on req.accepts(), but the LLM just wrote false information in docs and PR description - it sounds as if the author completely rewrote the method to extend the functionality, while in reality the changes to code were just cosmetic.
  3. (2025-12-02) docs: fix JSDoc for req.accepts() return value and parameter format #6936 is opened with a partial fix to the req.accepts() docs.
  4. (2026-01-06) I noticed that docs: fix JSDoc for req.accepts() return value and parameter format #6936 missed some spots in the docs.
  5. (2026-01-07) fix: enhance req.acceptsCharsets method #6088 is merged - the wrong req.acceptsCharsets() docs become a part of Express.
  6. (2026-01-08) docs: fix JSDoc for req.accepts() return value and parameter format #6936 is updated to eliminate the rest of mistakes in req.accepts() docs.
  7. (2026-01-16) @bjohansebas finds the reason why there was a mistake in the first place and merges docs: fix JSDoc for req.accepts() return value and parameter format #6936.

Conclusions? Maybe I should check PRs after they're merged if I've ignored them before...

TL;DR req.acceptsCharsets JSDoc comment claimed support for something that has not been supported since Express 4 and an LLM (user?) tried to "fix" it in code.

Description

req.accepts, req.acceptsEncodings, req.acceptsCharsets, req.acceptsLanguages are almost identical so I tried to:

  • make the JSDoc comments consistent,
    • mention all possible argument types:
      • single string (returns false or the string),
      • multiple string arguments (returns false or the best/preferred),
      • an array of strings (returns false or the best/preferred),
      • no arguments (returns an array of supported thing sorted by preference);
    • add examples showing most of the above cases (argument types, return values),
    • add overloads that better reflect the behaviour (thought that it may help, but now I have doubts about this);
  • change the methods to the same code style (based on Refactor: simplify acceptsLanguages implementation using spread operator #6137),
  • test the things the new docs claim are supported.

Problems and things to discuss

  1. The overloads probably are not necessary (the text content explains everything and the DefinitelyTyped types contain better docs/types). I would like to hear someone's opinion on this.
  2. Code style and docs format were my arbitrary choice and probably could be better.
  3. The tests are still a chaos (different structure in every file), but I don't plan to change this.
  4. 2 tests fail, because I have written them based on RFC 9110 and the behaviour of negotiator/accepts/express when Accept-Encoding is missing does not match my interpretation of the standard - No Accept-Encoding defaults to only 'identity' jshttp/accepts#82
  5. One test deletes a header, because of a problem with superagent - [fix] Cannot unset Accept-Encoding forwardemail/superagent#1860
  6. req.acceptsCharsets now contains a note that the Accept-Charset header is deprecated - is it something that should be included in the docs?

If this is merged, the docs on the website should be updated too. On the website only req.acceptsLanguages() has String[] in return types.

TODO:

  • Check whether @param {...string} name works with TS (...string[] that I use now I think is incorrect JSDoc)

Closes #7470

This was written based on RFC 9110, but right now it doesn't work - jshttp/accepts#82.

Separate commit to make it easier to revert/drop.
Since first Express 3.0 alpha this function returns the accepted type (or false). true was returned in versions of Express prior to 3.x.

The behaviour change occurred on 2012-03-24 (in multiple commits), but, because of how these tests were written, it did not affect tests even when the return type changed.

expressjs@365a98d
expressjs@86a9e08
expressjs@298899d
…not present

The new acceptsEncodings test will fail. I consider this to be a bug in jshttp/accepts.

jshttp/accepts#83 (comment)
That's the form `req.acceptsLanguages` was already using.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentations issues tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant