-
Notifications
You must be signed in to change notification settings - Fork 776
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
ensureFile misleading error message #696
Comments
Agreed, |
@RyanZim do you want a PR for this? @jprichardson has asked me to help out with this project and I'd like to start on something simple to familiarise myself with the codebase a little. |
@lukechilds sure, go ahead! |
I think this is already fixed, just tried writing a test to target the behaviour described in this issue but it fails:
I'm not getting Also noticed in the original issue:
The current version is 8.0.1. @TanninOne can you try upgrading to |
No, I still get the error exactly as I described with fs-extra@8.0.1 The same is true in the async code except it calls "pathExists" which uses fs.access to check for the existence of "foo" but doesn't verify it's a directory. |
@TanninOne thanks for the quick response. I totally missed that you were on Windows. This test is passing for me on macOS and Linux: https://travis-ci.com/lukechilds/node-fs-extra/builds/116252408 Is this test accurately describing your problem? it('should give clear error if node in directory tree is a file', () => {
const existingFile = path.join(TEST_DIR, Math.random() + 'ts-e', Math.random() + '.txt')
fse.mkdirsSync(path.dirname(existingFile))
fs.writeFileSync(existingFile)
const file = path.join(existingFile, Math.random() + '.txt')
try {
fse.createFileSync(file)
assert.fail()
} catch (err) {
assert.strictEqual(err.code, 'ENOTDIR')
}
}) e.g The test should fail with the behaviour you're experiencing but pass once it's resolved? Or am I misunderstanding the issue? I'll spin up a Windows VM and test in there. |
Ok, had some issues getting a Windows VM working but tested a Windows build on AppVeyor and it fails. So does indeed seem to be a Windows only problem: https://ci.appveyor.com/project/lukechilds/node-fs-extra/builds/25413529/job/m231uubewxg78kf0 |
Yes, the test reproduces the issue. Thank you for looking into this! |
Fixed in #744; will be released in 9.0.0. |
fs-extra
version: 4.0.3Say you have a file /foo/bar.txt
and call ensureFile('/foo/bar.txt/narf.txt') the error message you get is ENOENT for the path /foo/bar.txt/narf.txt which is confusing, because the purpose of the call is to create the file, why is it failing with the file being missing?
The cause is that pathExists (or fs.exists) only tests for the existence of the filesystem object, it doesn't verify whether it's a directory, so in this case the function proceeds to try to create /foo/bar.txt/narf.txt even though bar.txt is not a directory.
I think instead the error message should be EEXIST with path /foo/bar.txt - signaling that there is a file "bar.txt" that can't be replaced with the required intermediate directory required.
I do realize this is a minor thing but I feel like good error messages are important to quickly identify issues.
The text was updated successfully, but these errors were encountered: