-
Notifications
You must be signed in to change notification settings - Fork 0
fs: refactor unlinksync logic in rimraf #1
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
base: main
Are you sure you want to change the base?
Conversation
08e79f9
to
492d099
Compare
@anonrig I updated the PR with a TODO and description. I propose we move rimraf related logic to a separate |
83869f8
to
c70dd8d
Compare
@anonrig mind taking a look at the partial port of FixWinEpermSync for some feedback? I was going to ask for your advice on what is the best way to implement passing of error reference to callees in cpp and defer throwing like in the current behavior. Would creating an instance of |
return result == UV_ENOENT; | ||
} | ||
|
||
static void FixWINEPERMSync(char* path, uint32_t max_retries, uint32_t retry_delay) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning void makes it harder to keep track if this function errored or not. I recommend a boolean or std::optional if you need to pass a value to caller
Refactor and port rimraf functionality to c++
Since a lot of the functions (besides _unlinkSync) in rimraf.js directly call wrapper functions around fs defined in rimraf.js, small refactoring would require us to write a lot of code that would ultimately get discarded. It will be easier to review the refactored codebase function by function (ideally commit by commit) in the end and avoid having to keep track of intermediary signatures to handle JS -> C++ communication.
TODO:
rimraf
rimrafSync
rimrafPromises
_rimraf
_rmdir
_rmdirSync
_rmchildren
_unlinkSync
fixWinEPERM
fixWinEPERMSync
Once above functions are ported to C++, the dependency on rimraf.js should be removed the lazy loading logic associated with it should be removed from
lib/fs.js
. The final rimraf module should only expose rimraf and rimrafSync.