Fix: bug fixes and optimizations - #7404
Conversation
krzysdz
left a comment
There was a problem hiding this comment.
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?
|
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:
Discarded Performance Claims: • Lazy Request Getter Caching: Discarded (duplicate of #6042). Thanks again! |
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