Fix issue where subfolders aren’t checked for existence before creation

[blechatellier]

Getting an error when trying to save the store to /data/store, if a subfolder already exists the mkdir method doesn't check for it and throw an error.

Error: EEXIST: file already exists, mkdir '/'
    at Object.fs.mkdirSync (fs.js:885:18)
    at make (/cilia/node_modules/data-store/index.js:369:26)
    at mkdir (/cilia/node_modules/data-store/index.js:372:7)
    at Store.writeFile (/cilia/node_modules/data-store/index.js:288:5)
    at ontimeout (timers.js:475:11)
    at tryOnTimeout (timers.js:310:5)
    at Timer.listOnTimeout (timers.js:270:5)

[blechatellier]

Any idea why the CI failed on node 10? Probably un-related to my change.

[sant123]

Tested on Debian 8 and Node 10.6.0

¿May you re-run the CI again?

[doowb]

@blechatellier is /data/store a file on your system?

[doowb]

@sant123 thanks for the additional information. It looks like the tests are timing out on the CI so probably just need to re-run them, but I'd also like to find out what the root cause of the original issue is because EEXIST is handled when the subfolder is a directory.

[blechatellier]

/data is, expecting the store subfolder to be created but fails on /

[blechatellier]

@doowb re-ran the CI, checks are all good now, any chance to get this merged in please?

[jonschlinkert]

/data is, expecting the store subfolder to be created but fails on /

I believe @doowb is asking if /data/store/ is a file and not a directory. If so, that should throw an error, since you can't overwrite a file with a directory.

/data is, expecting the store subfolder to be created but fails on /

That's not what the error indicates. EEXIST means that something already exists, and is probably a file. So you might be seeing a false-positive. By using fs.existsSync(), you are bypassing the error handling that specifically ensures that you won't accidentally overwrite a file. In other words, if you're not getting an error now, it's probably because you're overwriting the file "successfully".

edit: to be clear, I'm not saying "this is definitely what's happening", I'm mentioning it in case it's what's happening. That said, even if it's not what's happening in this case, using fs.existsSync(filepath) will definitely cause this problem in other cases, unless we also do fs.statSync().isDirectory(), but then we'll also need to change other logic. My hunch is that it's an existing file (or was), and if it's not, then something else is going on that isn't clear yet.

[blechatellier]

@jonschlinkert got it, to give some context, I'm using data-store in a Docker container with a mounted volume at /data/store, definitely a directory and not a file but maybe why I'm seeing that false-positive?

[jonschlinkert]

definitely a directory and not a file but maybe why I'm seeing that false-positive?

It sounds like that's what's happening. Based on that, is your opinion still that is this definitely a bug? If so I think we might need to do more checking than fs.existsSync().

edit: I'm looking at it now, I have some ideas if you do think we need to make some changes, but I'll probably need more feedback after we try some things out to make sure it will work on other operating systems besides osx and windows.

[blechatellier]

Wouldn't say it's a bug but more of a corner case not handled, what other checks do you have in mind?

[jonschlinkert]

sorry for the late reply.

Wouldn't say it's a bug but more of a corner case not handled, what other checks do you have in mind?

I just pushed up some minor changes: 88c0d6f.

If you have a moment, please try that code out and let me know if it resolves the issue.

[blechatellier]

@jonschlinkert awesome, works - thanks!