Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs.rmdir looks not support "recursive" #292

Closed
kunyan opened this issue Apr 8, 2020 · 6 comments · Fixed by #293
Closed

fs.rmdir looks not support "recursive" #292

kunyan opened this issue Apr 8, 2020 · 6 comments · Fixed by #293
Assignees

Comments

@kunyan
Copy link

kunyan commented Apr 8, 2020

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
Copy link
Collaborator

3cp commented Apr 8, 2020

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 :-)

@3cp 3cp self-assigned this Apr 8, 2020
@kunyan
Copy link
Author

kunyan commented Apr 10, 2020

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

return this._mockedBinding[key].apply(this, arguments);

I think there has some difference in Node v12 ?

@3cp
Copy link
Collaborator

3cp commented Apr 19, 2020

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 added a commit that referenced this issue Apr 20, 2020
There is missing support of "path <string> | <Buffer> | <URL>" on many fs APIs.

closes #292
@3cp
Copy link
Collaborator

3cp commented Apr 20, 2020

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.

3cp added a commit that referenced this issue Apr 20, 2020
There is missing support of "path <string> | <Buffer> | <URL>" on many fs APIs.

closes #292
3cp added a commit that referenced this issue Apr 20, 2020
There is missing support of "path <string> | <Buffer> | <URL>" on many fs APIs.

closes #292
@kunyan
Copy link
Author

kunyan commented Apr 21, 2020

Please bump a new version for changes. thanks a lot

@tschaub
Copy link
Owner

tschaub commented Apr 24, 2020

Fix published in mock-fs@4.12.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants