Kikobeats / finepack

Organizes and maintains readable your JSON files.
MIT License
19 stars 7 forks source link

Verify file exists before trying to lint it #12

Open pdehaan opened 9 years ago

pdehaan commented 9 years ago

In typing up the previous bug report (#11), I mistyped the command as $ finepack package (without the ".json" extension because I had multiple package* files in my directory and I was lazily using CLI auto-complete). I got the following error:

$ finepack package # instead of the correct "package.json"

fs.js:439
  return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode);
                 ^
Error: ENOENT, no such file or directory '/Users/pdehaan/dev/tmp/finepack-test2/package'
  at Object.fs.openSync (fs.js:439:18)
  at Object.fs.readFileSync (fs.js:290:15)
  at Object.<anonymous> (/Users/pdehaan/.npm-packages/lib/node_modules/finepack/bin/index.js:31:19)
  at Module._compile (module.js:456:26)
  at Object.Module._extensions..js (module.js:474:10)
  at Module.load (module.js:356:32)
  at Function.Module._load (module.js:312:12)
  at Function.Module.runMain (module.js:497:10)
  at startup (node.js:119:16)
  at node.js:929:3

$ echo $?
8

If I'm reading it correctly, it's coming from here:

/* 29: */ var filepath = path.resolve(cli.input[0]);
/* 30: */ var filename = path.basename(filepath);
/* 31: */ var filedata = fs.readFileSync(filepath, {encoding: 'utf8'});

We may want to verify that the file exists before attempting to open it barfing all over the Terminal. Either checking if the file exists using fs.existsSync() or maybe wrapping the fs.readFileSync() in a try...catch block.


UPDATE: It looks like just wrapping the fs.readFileSync() in a try...catch block may be the answer (per Node docs):

fs.exists() is an anachronism and exists only for historical reasons. There should almost never be a reason to use it in your own code. In particular, checking if a file exists before opening it is an anti-pattern that leaves you vulnerable to race conditions: another process may remove the file between the calls to fs.exists() and fs.open(). Just open the file and handle the error when it's not there. fs.exists() will be deprecated. ... fs.existsSync() will be deprecated.

— via https://nodejs.org/api/fs.html#fs_fs_exists_path_callback

pdehaan commented 9 years ago

Not sure if relevant, but I'm using Node 0.10.35 and npm 1.4.28 apparently.

$ node -v; npm -v
v0.10.35
1.4.28
Kikobeats commented 9 years ago

so basically update the code and use fs.exists ?