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

process: chdir with callback implementation #870

Closed
wants to merge 1 commit into from
Closed

process: chdir with callback implementation #870

wants to merge 1 commit into from

Conversation

uxp
Copy link

@uxp uxp commented Feb 17, 2015

Add an optional callback argument to the process.chdir function which
enables execution of a closure in a directory different than the outer
scope. Also improve error messages that can be thrown and make sure
existing tests continue to pass.

Add an optional callback argument to the process.chdir function which
enables execution of a closure in a directory different than the outer
scope. Also improve error messages that can be thrown and make sure
existing tests continue to pass.
@uxp uxp closed this Feb 17, 2015
@uxp uxp reopened this Feb 17, 2015
@uxp
Copy link
Author

uxp commented Feb 17, 2015

Sorry for the extraneous open/close. The Contributor guidelines still reference that API changes should be merged into master, and only after review and possibly major/minor version bumps would it be merged into stable/v1.x, which caused some confusion as master has not been updated in some time.

With that out of the way, I don't fully expect this to be merged outright. I'm not especially fond of the way I implemented it, and am probably missing something major, as I'm not much of a C++ developer. If this appears to be something the io.js core team would like to be included in the project, I'd be more than happy to clean this up as needed, after review.

Thanks for your time.

@sam-github
Copy link
Contributor

This is basically

function chdir(path, callback) {
  var old = process.cwd();
  process.chdir(path);
  callback();
  process.chdir(old);
}

but implemented in C++, right?

As such, it seems pure sugar, best done in an external module, I don't see why it should be in core. Can you make a better case for why this needs to be in io.js, and not a user-land module?

Also, the API seems to promise asynchrony (it has a callback), but it is not in fact asynchronous, and could never become parallel either (since current working directory is part of global process state).

@cjihrig
Copy link
Contributor

cjihrig commented Feb 17, 2015

-1. This seems like it would lead to great user confusion.

@piscisaureus
Copy link
Contributor

-1:

  • The way the callback is used is completely different from any other node API.
  • I'd rather see uv_cwd() go away entirely. It's dangerous to use as it can create races with asynchronous file operations. What to do with uv_cwd() is also one of the open problems wrt multi-isolate support.

@uxp
Copy link
Author

uxp commented Feb 17, 2015

@sam-github That is correct, your example is pretty much exactly what I intended. Ruby and Perl have almost the same API, where the more global chdir (which should probably remain as process.chdir) is explicitly global, but File#chdir and File::chdir, respectively, allow a block to be executed independent of the main program localized to another directory. I've needed this kind of functionality a couple of times recently, and the fs module contained in Node/io.js lacks even basic directory management, leading to confusion for myself. Having to use process.chdir really isn't the way to go I admit, and altering an existing API feels so wrong I'm still quite embarrassed.

I tried to find a way to make this work as I intended, and since I know very little about the inner workings of Node/io.js, this is the only thing I was able to cobble together that let the existing tests pass and contain the behavior I intended, even if it's terribly wrong. Yes, I would like it to be an asynchronous operation, with the callback being common to node's API.

@cjihrig Can you explain further how this would lead to great user confusion? I don't disagree, but I would like more insight to your statement so I don't make the same mistake multiple times.

@uxp
Copy link
Author

uxp commented Feb 17, 2015

@sam-github As for the argument of this being in core vs an external module, I really have none except for that similar functionality exists in other languages core and standard libraries (Ruby and Perl, as described in my previous comment), and it feels like a lower level API rather than something implemented externally. Additionally, I've had a personal distaste for external libraries opening and altering core libraries, so creating a completely external module to support this behavior felt cluttering, and creating an extension library didn't feel right. I now realize it might make more sense as a new API within the fs module after writing these comments, but still wanted to at least bring up the discussion.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 17, 2015

@uxp it would confuse people because it is a synchronous function that appears to be asynchronous. It's also confusing because it's a synchronous chdir that actually leaves you in the original directory.

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.

4 participants