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

polyfill node's fs.cp. #6

Merged
merged 12 commits into from
Dec 9, 2021
Merged

polyfill node's fs.cp. #6

merged 12 commits into from
Dec 9, 2021

Conversation

everett1992
Copy link
Contributor

I need recursive copy to implement npm/cli#4082. I can probably use mkdir --recursive and paclist to copy module's files but I'll need cp recursive to copy modules in node_modules.

Like lib/rm/polyfill.js this is a copy/modification of code in nodejs, which is in turn lifted from a popular npm package (fs-extra in this case).

  • I may have gone overboard supporting various error classes.
  • I did not implement node's argument validation. I only enforce that opts is an object and set defaults.
  • I only copied the promise interface, not sync or callback.

@nlf
Copy link
Contributor

nlf commented Dec 8, 2021

i just landed #7 which reduces the CI test matrix a bit and makes that a little less scary (currently a lot of the failures in CI are due to testing on node versions that npm doesn't officially support)

can you rebase this on the latest main?

additionally, we'll need you to put the license for cp in a separate file at lib/cp/LICENSE.md

after those two changes we'll have a more clear picture of what still may need fixing, but judging by what i'm seeing we have at least one test that has failing assertions, and we're missing a line of coverage, both in Windows. i'm happy to help you track down how to fix those after a rebase!

I added this test trying to cover L158-163. It worked on linux and
covered the block but not windows.
@nlf
Copy link
Contributor

nlf commented Dec 8, 2021

awesome! one last thing, can you add a note to the README about this module providing an fs.cp polyfill? once that's in place i think we're good to land this. thank you!

Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

beautiful! thank you!

@nlf nlf merged commit fddb7cf into npm:main Dec 9, 2021
@nlf
Copy link
Contributor

nlf commented Dec 9, 2021

just published this as @npmcli/[email protected]

thank you again, this is great!

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.

2 participants