fix(basename): ignore trailing seperator#180
Conversation
src/path.ts
Outdated
| const lastSegment = normalizeWindowsPath(p) | ||
| .replace(/\/+$/, "") | ||
| .split("/") | ||
| .pop(); |
There was a problem hiding this comment.
Perf: I think we can use two lines const segments = [] and another check to see if last part is empty, fallback to len-1 this way we avoid regex
There was a problem hiding this comment.
Perf: I think we can use two lines
const segments = []and another check to see if last part is empty, fallback to len-1 this way we avoid regex
We need to ignore all trailing seps if there are many, see my test cases (/foo/bar////). If we loop and pop, it will be slower than regex replace.
There was a problem hiding this comment.
I think most of the fast and normal conditions is that there is no trailing slash or just one. We don't need to pop, we can just have a reverse for loop to find the first index that has non-empty one.
Feel free to do a benchmark. What matters to me is that we don't add a perf overhead for normal conditions by adding regex replace (since this is a low level utility, it could be used hundreds or thousands of times in high level tools)
There was a problem hiding this comment.
@pi0 Benchmark: https://replit.com/@isukkaw/SandyPossibleLanguage
TL; DR:
endsWithbefore regex replace is fastest when the input doesn't end with sep. But it is slowest if there are any- reverse loop and pop is fastest if there are seps at the end of the input. It is the second fastest if the input doesn't end with one (but it is still 10x slower than
endsWithbefore regex replace)
So which one should we choose?
There was a problem hiding this comment.
Thanks!! See this updated repl. I have made few changes, removed other variants and add tests/adjustment that all implementations return basename for us and seems loop can be always fastest.
https://replit.com/@pi0/httpsgithubcomunjspathepull180
How do you think?
local (bun)
local (node 20)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #180 +/- ##
==========================================
- Coverage 99.45% 99.28% -0.17%
==========================================
Files 4 4
Lines 364 281 -83
Branches 114 116 +2
==========================================
- Hits 362 279 -83
Misses 2 2 ☔ View full report in Codecov by Sentry. |
Fixes #179.