eldargab / node-fake-fs

Fake node.js file system for testing
32 stars 5 forks source link

fake-fs should probably use 'bind' to fix the 'this' of its 'node-like' methods. #5

Closed kdvolder closed 11 years ago

kdvolder commented 11 years ago

The node-like filesystem api produced by

var fs = new Fs()

Is not quite equivalent to node fs in the sense that code like the following, which works with node fs doesn't work with a fake fs.

var fs = new Fs();
var readFile = fs.readFile;
readFile('/foo/bar.txt', ...);

This code breaks because the 'this' of readFile call is incorrectly set. It only works correctly if I call it like so: fs.readFile(...).

With 'raw' nodefs I can either call fs.readFile(...) or just readFile(...). Both are equivalent.

That means I have to be a tad more careful how I use/call the function from a fake fs instance. Ideally a fake fs instance should as much as possible try to emulate the behavior of raw node fs.

So, I wonder if it would be possible for the fake constructor to automatically 'seal' the returned api and bind the this object automatically for the node-like methods in the object returned by new Fs().

eldargab commented 11 years ago

Makes sense. I agree in general because I can't give any argument against, except, may be, performance. Haha, sounds crazy, but such binding things are extremely slow, so, who knows? :) Also once you know that fake fs is a classic object (which is not so unexpected BTW) it's not hard to avoid var readFile = fs.readFile. Also, personally, I only used fake-fs for patching global fs module (that's because of third party dependencies). That's why I'd like to leave things as is for now.

kdvolder commented 11 years ago

It is entire your call :-)

I'm using fake-fs to test a 'composable' filesystem I'm developing. The idea is you can create something that looks and behaves like nodefs by plugging together different things that provide a way to read/write files and directories.

So you can do something like:

  var nodefs = require('fs');
  var myFs = compose(
       withPrefix('/foo', subDir('/home/kdvolder/foo', nodefs)),
       withPrefix('/tmp', subDir('/tmp', nodefs))
  );

To create regression tests for this I'm using 'fake-fs' instances instead of nodefs.

In a way, actually the fact that 'fake-fs' doesn't 'auto-bind' the functions may be good for my tests. If my composite-fs implementation doesn't take care of proper binding or this arguments, then it will still work for nodefs but it may not work for other potential fs implementations that don't 'auto-bind'.

Now that my code is tested to behave with fakefs it will also work with nodefs, but the other way around may not be true. So in a way having fake-fs not do the 'auto-bind' while it deviates from standard nodefs behavior may actually make it a better test :-)

Anyway, I guess I just made a pretty good argument not to do auto-binding :-)

I won't be offended at all if you close this issue.

eldargab commented 11 years ago

Yeah, I'll remember the argument :)