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

Always throw if unable to overwrite #38

Merged
merged 2 commits into from Feb 24, 2020

Conversation

boneskull
Copy link
Contributor

@boneskull boneskull commented Oct 24, 2019

This is a breaking change

Fixes #37

test/async.js Show resolved Hide resolved
test/sync.js Show resolved Hide resolved
@boneskull
Copy link
Contributor Author

Maybe more appropriate to say "always throw (or reject) on copy failure"

@boneskull
Copy link
Contributor Author

node v12.13.0 introduced a breaking change, I'll investigate

@boneskull
Copy link
Contributor Author

I've added a change to work around the issue (version sniffing! fun!)

@boneskull
Copy link
Contributor Author

@sindresorhus You may want to comment on nodejs/node#29930 as I don't think I can be very much help

@mcollina
Copy link

I think you should wait for open to be emitted on the writable stream as well before piping. I’m not sure why this was not causing problem before, as it seems a race condition to me between the readable and writable sides.

@sindresorhus sindresorhus changed the title always throw if unable to overwrite; closes #37 Always throw if unable to overwrite Nov 12, 2019
@boneskull
Copy link
Contributor Author

I'm not sure what's going on here.

@sindresorhus
Copy link
Owner

@boneskull It's not clear to me what the status of this PR is?

@boneskull
Copy link
Contributor Author

I’m not sure what to do with @matteocollina’s feedback. there’s a workaround in this PR, and afaik the PR works, but I’m unsure if I should be changing something else per his feedback

@sindresorhus
Copy link
Owner

@mcollina The docs says nothing about having to wait for an open event: https://nodejs.org/api/fs.html#fs_fs_createwritestream_path_options

@sindresorhus sindresorhus merged commit 4668c5a into sindresorhus:master Feb 24, 2020
sindresorhus added a commit that referenced this pull request Feb 24, 2020
@sindresorhus
Copy link
Owner

I've added it just in case though: 6d92d5c

@sindresorhus
Copy link
Owner

Reverted that ^. It caused problems on Node.js 12 (I apparently had an older 12 version locally so I didn't detect the problem before pushing). https://travis-ci.org/sindresorhus/cp-file/jobs/654317803?utm_medium=notification&utm_source=email

@boneskull boneskull deleted the issue/37 branch June 15, 2021 19:15
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.

need feedback if copy fails because overwrite: false
3 participants