Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(basename): ignore trailing seperator #180

Merged
merged 4 commits into from
Dec 30, 2024
Merged

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Aug 7, 2024

Fixes #179.

@pi0 pi0 changed the title fix(#179): ignore trailing sep in basename fix(basename): ignore trailing seperator Aug 7, 2024
src/path.ts Outdated
const lastSegment = normalizeWindowsPath(p)
.replace(/\/+$/, "")
.split("/")
.pop();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@SukkaW SukkaW Aug 7, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@SukkaW SukkaW Aug 7, 2024

Choose a reason for hiding this comment

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

@pi0 Benchmark: https://replit.com/@isukkaw/SandyPossibleLanguage

TL; DR:

  • endsWith before 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 endsWith before regex replace)

So which one should we choose?

Copy link
Member

@pi0 pi0 Aug 7, 2024

Choose a reason for hiding this comment

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

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)

image

local (node 20)

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c5b922e (#180)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pi0 Just a friendly ping~

@SukkaW SukkaW requested a review from pi0 August 7, 2024 10:47
Copy link

codecov bot commented Dec 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.28%. Comparing base (bdab761) to head (1a2087c).
Report is 12 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

❤️

@pi0 pi0 merged commit 7534c0c into unjs:main Dec 30, 2024
2 of 3 checks passed
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.

pathe.basename behaves differently w/ path.basename when handling trailing separator
2 participants