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

Provide 'EEXIST' code for "already exists" errors #661

Closed
wants to merge 1 commit into from

Conversation

urugator
Copy link

@urugator urugator commented Mar 4, 2019

So we don't have to match against the error message.

Related: #461

@RyanZim
Copy link
Collaborator

RyanZim commented May 14, 2019

Closing for the same reasons as #592.

@RyanZim RyanZim closed this May 14, 2019
@urugator
Copy link
Author

urugator commented May 15, 2019

Can't we pass fs.constants.COPYFILE_EXCL flag to fs.copyFile when overwrite === false and catch/rethrow the native error based on errorOnExist?
It would:

  • make the existence check atomic (current check isn't safe in concurrent env)
  • provide a way to handle the error meaningfully

@RyanZim
Copy link
Collaborator

RyanZim commented May 15, 2019

The problem with that is that we still need to support environments that don't support fs.copyFile.

@urugator
Copy link
Author

The fallback uses createWriteStream:

The exclusive flag 'x' (O_EXCL flag in open(2)) ensures that path is newly created. On POSIX systems, path is considered to exist even if it is a symlink to a non-existent file. The exclusive flag may or may not work with network file systems.

Is that viable?

What is the suggested way to handle errorOnExist error?
I mean, the existence check isn't reliable enough as a guarantee for data loss prevention.
But it's also hard to use as a naive "let the user know" check, because the error is "indistinguishable" from others.
So what's the use case? Should the overwrite and errorOnExists option even exist?

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