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

Require Node.js 8, detect read IO errors after open #33

Merged
merged 2 commits into from
Apr 19, 2019
Merged

Require Node.js 8, detect read IO errors after open #33

merged 2 commits into from
Apr 19, 2019

Conversation

coreyfarrell
Copy link
Contributor

@coreyfarrell coreyfarrell commented Apr 12, 2019

@sindresorhus if you prefer I can drop the second commit from this PR, resubmit separately if/when the first is merged. First commit upgrades everything to the node.js 8 way (technically this requires node.js 8.5 for fs.copyFileSync).

Second commit addresses the existing issue where read IO errors after open were not processed. In addition to the automated test I manually tested by unplugging a USB drive in the middle of copying a Linux distro ISO from it (that's where I got the exact EIO error used in testing). If you don't want the first commit then @Henry128 can probably just copy my test into #31.

Fixes #30
Closes #31

* Drop pify in favor of util.promisify
* Use p-event to promisify streams
* Drop copySyncFallback and all related code
* Remove `default` export per TODO comments
index.js Show resolved Hide resolved
@sindresorhus sindresorhus merged commit 5e42b4a into sindresorhus:master Apr 19, 2019
@sindresorhus
Copy link
Owner

Hmm, this is failing on macOS: https://travis-ci.org/sindresorhus/cp-file/jobs/522134757

@coreyfarrell
Copy link
Contributor Author

I remember having an issue with EACCES testing outside linux on another package, can't quite remember where though (I'm pretty sure it was one of your packages though). I'll look at my PR history see if I can find it.

@coreyfarrell
Copy link
Contributor Author

@sindresorhus https://github.com/sindresorhus/path-type/pull/3/files#diff-6624e890b4dd46cf165d3bc6591ee9c0 - looks like I just hijacked fs methods to throw EACCES. I can post another PR to move the EACCES test from this module into a separate test file and do the same here?

@coreyfarrell coreyfarrell deleted the node8 branch April 19, 2019 12:46
@sindresorhus
Copy link
Owner

I can post another PR to move the EACCES test from this module into a separate test file and do the same here?

👍

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.

If source file is removed/errored (Like an SD card pulled out) promise never resolves.
2 participants