tschaub / mock-fs

Configurable mock for the fs module
https://npmjs.org/package/mock-fs
Other
911 stars 86 forks source link

fs.rmdir looks not support "recursive" #292

Closed kunyan closed 4 years ago

kunyan commented 4 years ago
fs.rmdir("/some/path", {recursive: true })

Will get error

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received an instance of Buffer
3cp commented 4 years ago

You are right, recursive is only implemented for mkdir now. I will try to implement it when I got time. But you are welcome to contribute :-)

kunyan commented 4 years ago

I'm trying to implement this feature in my local, however. the arguments in Binding.prototype.rmdir always not have new arg when I run ./test/lib/fs.rmdir.spec.js

And I also found where the error message coming from

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received an instance of Buffer

The arguments[0], path will be convert to a Buffer in readdir https://github.com/tschaub/mock-fs/blob/9832d66e09cd49b60a55b5c8272e01dc67ca64b1/lib/index.js#L35

I think there has some difference in Node v12 ?

3cp commented 4 years ago

From nodejs source code

function lazyLoadRimraf() {
  if (rimraf === undefined)
    ({ rimraf, rimrafSync } = require('internal/fs/rimraf'));
}

function rmdir(path, options, callback) {
  if (typeof options === 'function') {
    callback = options;
    options = undefined;
  }

  callback = makeCallback(callback);
  path = pathModule.toNamespacedPath(getValidatedPath(path));
  options = validateRmdirOptions(options);

  if (options.recursive) {
    lazyLoadRimraf();
    return rimraf(path, options, callback);
  }

  const req = new FSReqCallback();
  req.oncomplete = callback;
  binding.rmdir(path, req);
}

The binding.rmdir (where you expect the new option) does not handle this new option, but a newly created rimraf (from 'internal/fs/rimraf') does some wrap around other fs and binding calls to implement the recursive deleting.

I will try to glue them up.

3cp commented 4 years ago

Because nodejs v12 implemented recursive rmdir though 'internal/fs/rimraf', there is nothing we need here.

The broken support was due to a bug of mock-fs: not supporting Buffer typed path input. More details in #293.

kunyan commented 4 years ago

Please bump a new version for changes. thanks a lot

tschaub commented 4 years ago

Fix published in mock-fs@4.12.0.