-
Notifications
You must be signed in to change notification settings - Fork 122
feat: Implement robust browser path resolution #505
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
base: main
Are you sure you want to change the base?
feat: Implement robust browser path resolution #505
Conversation
This module is independent from npm dependencies and live in `vendor` directory. As browser only support importing ES modules, I've changed a bit the module exports to use ESM instead. The `path-browserify` package matches with Node.js v10.3 API.
- Updated the `normalize` and `resolve` function to use equivalent functions from `path-browserify` package
|
To understand a bit more about what this would allow. Is |
|
Hi @shadowspawn, I understand that explicit path normalization/resolution might seem niche for browser environments, where there's no direct file system interaction. However, the intent behind this PR isn't to enable file system operations in the browser, but to address a known TODO in Even in a browser, arguments often look like file paths (e.g.,
This ensures that Hope this clarifies the intended use case! |
|
To be clear, I'm not personally using |
|
Interesting to see you found a module for this very purpose. My initial thought is that vendoring in I was interested to see that according to the path-browserify README, it was included by default in some bundlers. If that is still a common pattern, a light weight approach might be to look for an available implementation supplied by the bundler.
Another approach would be to make it possible to supply own implementations so end-user can implement it if needed. It would be much smaller to only vendor the relevant routines. I am only one maintainer! I suggest you wait for feedback from other maintainers or end users before doing more work on this. |
|
Thank you for the clear guidance. I understand your concerns about the overhead of the current vendoring approach for |
This PR addresses the long-standing
TODOitem inbrowser.jsby integratingpath-browserifyto provide robust path normalization and resolution capabilities for browser environments.Previously,
browser.jsrelied on no-op implementations fornormalizeandresolve.yargs-parser/browser.js
Lines 7 to 16 in 553c808
After integrating
path-browserify, thenormalizeandresolvefunctions have been updated to directly referencepath-browserify.normalizeandpath-browserify.resolve, respectively.yargs-parser/browser.js
Lines 6 to 18 in 08254e3
Implementation
Integration of
path-browserifyas a Vendored Modulepath-browserifylibrary, which offers a browser-compatible API mirroring Node.js'spathmodule (specifically Node.js v10.3), has been included directly within thevendor/path-browserifydirectory.path-browserify-1.0.1.tgz) usingwget.path-browserify's primary entry point (index.js) has been slightly modified to use ES module exports (export default posix;).yargs-parser/vendor/path-browserify/index.js
Lines 527 to 529 in 08254e3
browser.jsPath Function Updatenormalizeandresolvefunctions within theYargsParserconstructor inbrowser.jsnow directly referencepath-browserify.normalizeandpath-browserify.resolve, leveraging the new capabilities.Build and Linting Adjustments
.eslintignorefile has been reconfigured to ignore thevendordirectory during linting.vendordirectory, ensuringpath-browserifyis correctly bundled into the final distribution.Justification for Dependency Management Strategy (Manual Vendoring)
The decision to manually vendor
path-browserify(by fetching the1.0.1.tgzdirectly from the npm registry) rather than adding it as a standardnpmdependency was made for the following reasons:path-browserifypackage, while functionally complete for this purpose, has not received updates on npm for a number of years. Manually vendoring allows for precise control over the exact version included inyargs-parser's browser bundle, isolating it from potential futurenpmdependency changes or deprecations.path-browserify'sindex.jsto expose ES module exports, which is crucial forbrowser.js's current import structure.Testing
I am keen to discuss these considerations and explore any preferred alternative approaches to integrating Browser version of Node.js
pathmodule.