[bug] functions readFile and writeFile execute callbacks even if there are errors/missing args (not matching fs behaviour)

Hi all! I'm working on an automated test generator and have been testing it by applying regression testing to a set of popular npm libraries, including yours! In this commit I found a difference in the behaviour of readFile and writeFile when called with incorrect arguments.

The issue

Functions that have been converted with universalify.fromPromise execute the callback argument even when the other arguments are missing or invalid. It looks like this results in a bug, because the comparable fs functions do error checking of these arguments.

To replicate the issue:

let jsonfile = require('jsonfile'');
function callback() { 
  console.log("Callback executing!");
  console.log(arguments);
}
jsonfile.readFile("existingFile.js", callback); // correct call
jsonfile.readFile( callback); // incorrect call

Before the change in this commit, the output of running the above code is:

// correct call output: 
> Callback executing!
> [Arguments] { '0': null, '1': 'file contents' }
// incorrect call output:
> Uncaught:
TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance of Buffer or URL. Received function callback
    at readFile (fs.js:326:10)
...

Then, after the update, the output of running the above code is:

// correct call output:
> Callback executing!
> [Arguments] { '0': null, '1': 'file contents' }
// incorrect call output:
> Callback executing!
> [Arguments] {                                                                                                                                                                
  '0': TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance of Buffer or URL. Received undefined                                        
      at readFile (fs.js:326:10)  
...

So, the callback is now executed even when the arguments are incorrect, and have the function error passed in as an argument to the callback.

What is causing this behaviour

This change from using the universalify library’s fromPromise function instead of fromCallback is causing the difference.

Looking at the implementation of fromPromise: this function takes a function fn as an argument, and returns a new function that

calls fn as it would normally be called, if the last argument is not a callback
if the last argument is a callback, then fn is called with all but its last argument (this is fn.apply(this, args.slice(0, -1))). As stated in the documentation, the fromPromise function assumes that fn returns a promise. Then, it chains a .then to the return of the call, passing the function r => cb(null, r) as the resolve of this .then promise, and cb (the callback argument itself) as the reject function.

In our case, we see that the incorrect call to readFile does result in an error TypeError[ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance of Buffer or URL.. And so, the promise resulting from the call to readFile is rejected. As such, the reject branch of the .then is triggered, which results in our callback being executed.

We see that in our callback, the TypeError is printed as it is the argument to the callback.

`writeFile`: another function with this behaviour

The writeFile function is also universalified with fromPromise. As such, calling writeFile with only a callback has the same behaviour as readFile (i.e. the callback is executed).

jsonfile.writeFile("file.json","output string", callback); // correct call
> Callback executing!
> [Arguments] { '0': null }

// incorrect call still executes the callback
jsonfile.writeFile( callback); 
> Callback executing!
>  [Arguments] {                                                                                                                                                                
  '0': TypeError: Cannot read property 'replace' of undefined                                                                                                                  
      at stringify
...

Comparison to behaviour of corresponding 'fs' functions

I assume writeFile and readFile should have similar error checking to fs.writeFile and fs.readFile respectively; both of these functions have a type error if you try and call them with just a callback.

let fs = require('fs');
fs.writeFile(callback); // this function expects 3 arguments, and the last must be a callback
> Uncaught:                                                                                                                                                                            
TypeError [ERR_INVALID_CALLBACK]: Callback must be a function. Received undefined                                                                                                    
    at maybeCallback (fs.js:160:9)
    at Object.writeFile (fs.js:1431:14)

fs.readFile(callback);
Uncaught:
TypeError [ERR_INVALID_CALLBACK]: Callback must be a function. Received undefined
    at maybeCallback (fs.js:160:9)
    at Object.readFile (fs.js:311:14)
...

Fix

It is not enough to just add argument type checking code to the implementations of readFile and writeFile: because of the design of fromPromise, the callback will be executed regardless of the contents of these functions. I have a proposed fix that simply involves replicating the implementation of fromPromise in the body of these functions, after some type checking. I'll make a PR soon!

jprichardson / node-jsonfile