-
Notifications
You must be signed in to change notification settings - Fork 632
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
feat(path): single file exports #3510
feat(path): single file exports #3510
Conversation
Move to single export file structure looks good to me, and it's probably less controversial to other points if it doesn't have breaking changes. Can we only do the move to single-file export without API design changes in this PR?
These points seem needing more discussion, also these 2 changes don't seem necessarily in the same PR. Maybe try them later PRs? |
I guess you're right, but I just wanted to bundle breaking changes together. I'm not sure what the best way to move forward with single-file exports is. Should we expose |
I think we can also update
I don't think so. The names sound strange to me. I don't think we should deprecate |
Also can you add the license headers |
Alright, un-deprecating |
I guess this module will have quite the turbulent future. This should no longer be a breaking change (even if I personally think it should be). We now have duplicated implementations between |
Updated the original comment for future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sidenote: each function in this sub-module should have JSDocs. I guess this can be done in a follow-up PR.
With #3515, is there any value in keeping |
It looks like in most test cases, the APIs are called via Can you also replace the APIs under path.posix and path.win32 with the new ones? I think each platform specific function (like |
I would need to export |
Alternatively, which is what I initially pushed for, we could implement an I don't think we should make our API surface larger over this, especially with the naming as it is. |
I agree. How about moving platform specific implementations of, for example, |
That seems reasonable. I'll go ahead with that. |
I was skeptical at first, but I'm now convinced that this was the way to do it. Thanks for the suggestion @kt3k! This should now really be ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits. Otherwise, LGTM! Nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice work! @lino-levan
I'd tag this change as feat
as newly available paths can be considered as new features.
Related #687.
Future PRs: