Skip to content

Fix: bug fixes and optimizations - #7404

Closed
spellsaif wants to merge 2 commits into
expressjs:masterfrom
spellsaif:fix/bug-fixes-and-optimizations
Closed

Fix: bug fixes and optimizations#7404
spellsaif wants to merge 2 commits into
expressjs:masterfrom
spellsaif:fix/bug-fixes-and-optimizations

Conversation

@spellsaif

Copy link
Copy Markdown

This pull request addresses a bug, corrects outdated documentation, and introduces several non-breaking performance optimizations and modern feature supports to the Express prototype:

1. Bug Fixes & Correctness

• Custom Redirect Status Codes (res.redirect): Fixed a bug where redirects using non-standard status codes (e.g. 399) returned "undefined. Redirecting to..." due to statuses. message[status] returning undefined. It now gracefully falls back to "Redirecting".
• Stale Documentation (res.location): Updated outdated comments implying "back" is still a magic string. Support was removed in Express 5, but the JSDoc wasn't cleaned up.

2. Performance Optimizations

• Lazy Request Getter Caching (req.*): Cached values inside lazy getters on req (including query, protocol, ip, ips, subdomains, path, host, hostname, and xhr). Accessing these properties repeatedly now avoids duplicate query string parsing and network/proxy lookup logic.
• Heap Allocation Avoidance (res.send): Avoids allocating a Buffer on the heap when calculating Content-Length for string payloads ≥1000 characters if ETag generation is disabled. It now relies entirely on Buffer.byteLength(chunk, encoding).

3. ES6+ Compatibility & Robustness

• BigInt / Symbol Support (res.send, res.cookie): Added proper serialization/handling for BigInt (coercing to string in res.send and converting inside cookie maxAge) and clean TypeError propagation for Symbol/Function payloads, rather than crashing inside Node.js core Buffer.from.

Verification & Testing

• New Tests Added:
• Getter caching verification on request prototype.
• BigInt and Symbol behavior on res.send().
• BigInt option validation on res.cookie().
• Non-standard redirect status fallback check.
• Test Results: All tests pass successfully (npm test).
• Linter Status: Checked and verified with zero violations (npm run lint).

I certify that the contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license, and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license.

Signed-off-by: spellsaif

@krzysdz krzysdz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't submit PRs which combine multiple completely unrelated changes. This makes it harder to review and commit history becomes less useful.

When claiming performance improvements, please include benchmark results and benchmark code to verify.

• Custom Redirect Status Codes (res.redirect): Fixed a bug where redirects using non-standard status codes (e.g. 399) returned "undefined. Redirecting to..." due to statuses. message[status] returning undefined. It now gracefully falls back to "Redirecting".

Could you explain the use case? Some clients like curl will follow redirects with 3xx status codes, but others like Firefox will not respect unknown status codes.

• Stale Documentation (res.location): Updated outdated comments implying "back" is still a magic string. Support was removed in Express 5, but the JSDoc wasn't cleaned up.

Looks like not all references to "back" have been removed in #5933. Would you like to create a separate PR that updates the docs?

• Lazy Request Getter Caching (req.*): Cached values inside lazy getters on req (including query, protocol, ip, ips, subdomains, path, host, hostname, and xhr). Accessing these properties repeatedly now avoids duplicate query string parsing and network/proxy lookup logic.

Duplicate of #6042.

• Heap Allocation Avoidance (res.send): Avoids allocating a Buffer on the heap when calculating Content-Length for string payloads ≥1000 characters if ETag generation is disabled. It now relies entirely on Buffer.byteLength(chunk, encoding).

Did not analyse this in detail, but commit 48940e6 and the earlier comment convert chunk to Buffer; saves later double conversions (removed in the referenced commit) suggest that this was done for performance reasons. This is where benchmarks are necessary if you want to change something in the name of performance.

• BigInt / Symbol Support (res.send, res.cookie): Added proper serialization/handling for BigInt (coercing to string in res.send and converting inside cookie maxAge) and clean TypeError propagation for Symbol/Function payloads, rather than crashing inside Node.js core Buffer.from.

Converting BigInts in res.send() to strings makes more sense than the previously attempted #5445.
Converting BitInt to Number is lossy and you're probably doing something wrong if you pass a BigInt as maxAge.
I'm not sure if rejecting Symbols and Functions is necessary if Node.js does it. Sure, the error message may be slightly more user-friendly, but it does not change anything when it comes to functionality. Technically, Symbols and Functions can be converted to strings, so why reject them?

@spellsaif

spellsaif commented Aug 6, 2026

Copy link
Copy Markdown
Author

Hi @krzysdz ,

Thanks for the feedback! I've closed this PR and split the valid changes into three separate PRs. Since we discarded the performance-related claims, no benchmarks are included:

  1. JSDoc Cleanup (res.location): docs(res.location): clean up deprecated back string references #7406 — Cleaned up the leftover "back" comments.
  2. Custom Redirect Status Codes (res.redirect): fix(res.redirect): fallback to 'Redirecting' for non-standard status … #7407 — Falls back to "Redirecting" instead of printing "undefined" if a custom status code is used.
  3. BigInt Support (res.send): feat(res.send): add BigInt support #7408 — Coerces BigInts to strings in res.send(). We discarded the lossy maxAge conversion and redundant Symbol/Function checks.

Discarded Performance Claims:

• Lazy Request Getter Caching: Discarded (duplicate of #6042).
• Heap Allocation Avoidance: Discarded. Your point about 48940e6 is correct—encoding to a Buffer early avoids double-encoding when the string is written to the socket.

Thanks again!

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

Successfully merging this pull request may close these issues.

2 participants