Skip to content

Conversation

@Phillip9587
Copy link
Contributor

@Phillip9587 Phillip9587 commented Feb 11, 2025

This PR introduces a lightweight basename function to remove the dependency on node:path, enabling usage in environments like the browser. Additionally, new test cases have been added to validate the function's handling of various path formats.

Ref: expressjs/discussions#331

@Phillip9587 Phillip9587 mentioned this pull request Feb 12, 2025
@wesleytodd
Copy link
Member

While I understand the link to the discussion there, there are very popular polyfills for the path module for use in the browser, for example with webpack: https://v4.webpack.js.org/configuration/node/

I am not sure this actually wins us anything.

@Phillip9587
Copy link
Contributor Author

Phillip9587 commented Mar 5, 2025

@wesleytodd I understand your point. I thought about proposing pathe which is used for example by Nuxt or unenv but content-dispositions use case is so simple and only uses the basename function. Merging this PR would allow users to use this package for exampel with cloudflare's workerd runtime.

@wesleytodd
Copy link
Member

I think it is even supported there: https://developers.cloudflare.com/workers/runtime-apis/nodejs/path/

This one is one of the most ubiquitously supported ones because EVERYTHING imports it lol.

@bjohansebas
Copy link
Member

I'm neutral on this change. I wonder if this will break anything, given that several operating systems need to be considered, which is what Node would do for us

@Phillip9587
Copy link
Contributor Author

Thanks for the feedback!

I think it is even supported there: https://developers.cloudflare.com/workers/runtime-apis/nodejs/path/

Just to clarify, node:path is only available in Cloudflare’s Workerd runtime when the nodejs_compat or nodejs_compat_v2 flags are enabled. But that’s just one example - other runtimes may have different levels of support. For example, Vercel’s Edge Runtime does not support node:path: https://vercel.com/docs/functions/runtimes/edge#compatible-node.js-modules

This one is one of the most ubiquitously supported ones because EVERYTHING imports it lol.

Totally - path is everywhere. But we also need to consider browser compatibility. In environments like bundlers or edge runtimes targeting broader platforms, relying on path can lead to unnecessary overhead (due to full-module polyfills) or breakage unless polyfills are explicitly configured. Since content-disposition only uses basename, a lightweight replacement helps avoid these issues and makes the package more flexible across environments.

I'm neutral on this change. I wonder if this will break anything, given that several operating systems need to be considered, which is what Node would do for us.

Absolutely valid concern. That said, since we’re only using basename to strip everything before the last slash or backslash (to get the filename), the logic stays simple and OS-agnostic. I’ve added extensive tests to cover edge cases across platforms—if there’s anything I missed, I’m happy to add it.

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.

3 participants