Skip to content

Conversation

homersimpsons
Copy link

@homersimpsons homersimpsons commented Aug 29, 2025

Previous implementations shortcommings:

(_header || "")?.split(",").shift()?.trim();
         |     |           |       \- Useless as we have a string before `split`, so at least 1 string element in the array
         |     |           \- Useless as we can just get the first character `[0]`
         |     \- Useless as we cannot have undefined before
         \- Useless as we can just skip the whole computation if the header is falsy
  • Remove header fallback to empty string
  • Remove useless undefined check
  • Uses [0] instead of shifting

Note that I also tried to play with substring / slice and use split(',', 2) but that didn't seem to improve the performances.

Resolves #1196

perf.link reports that the new implementation is twice faster on a micro-benchmark

- Remove header fallback to empty string
- Remove useless undefined check
- Uses [0] instead of shifting
@homersimpsons homersimpsons requested a review from pi0 as a code owner August 29, 2025 16:07
Copy link

codecov bot commented Aug 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Comment on lines +377 to 382
if (_header) {
const xForwardedFor = _header.split(",")[0].trim();
if (xForwardedFor) {
return xForwardedFor;
}
}
Copy link
Author

Choose a reason for hiding this comment

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

If we want to code-golf this we can use &&, do you prefer that I write it like this?

Suggested change
if (_header) {
const xForwardedFor = _header.split(",")[0].trim();
if (xForwardedFor) {
return xForwardedFor;
}
}
const xForwardedFor = _header && _header.split(",")[0].trim();
if (xForwardedFor) {
return xForwardedFor;
}

(this may have a slight, but not noticeable impact, as in case _header is falsy it will have to be evaluated as falsy twice)

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.

getRequestIP implementation is suboptimal
1 participant