Skip to content

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

JonasBa
Copy link
Owner

@JonasBa JonasBa commented Oct 13, 2023

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.

@JonasBa JonasBa force-pushed the jb/rimraf/_unlinksync branch 4 times, most recently from 08e79f9 to 492d099 Compare October 14, 2023 03:15
@JonasBa
Copy link
Owner Author

JonasBa commented Oct 18, 2023

@anonrig I updated the PR with a TODO and description. I propose we move rimraf related logic to a separate node_file_rimraf.cc for the duration of the refactor (or indefinitely as it seems to have been isolated in that rimraf.js file anyways)

@JonasBa JonasBa force-pushed the jb/rimraf/_unlinksync branch from 83869f8 to c70dd8d Compare October 18, 2023 23:11
@JonasBa
Copy link
Owner Author

JonasBa commented Oct 22, 2023

@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 v8::Exception and passing its reference be the proper solution here?

return result == UV_ENOENT;
}

static void FixWINEPERMSync(char* path, uint32_t max_retries, uint32_t retry_delay) {
Copy link
Collaborator

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

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 this pull request may close these issues.

2 participants