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

Pull in sindresorhus/user-home as os.homedir() #682

Closed
vkurchatkin opened this issue Jan 31, 2015 · 13 comments
Closed

Pull in sindresorhus/user-home as os.homedir() #682

vkurchatkin opened this issue Jan 31, 2015 · 13 comments
Labels
os Issues and PRs related to the os subsystem.

Comments

@vkurchatkin
Copy link
Contributor

  1. This is generally useful
  2. We already have tmpdir()
  3. It is required for repl history support

/cc @a8m @sindresorhus

@rvagg
Copy link
Member

rvagg commented Jan 31, 2015

Anybody else feel like #596 is getting out of hand or am I the only one?

@cjihrig
Copy link
Contributor

cjihrig commented Jan 31, 2015

@vkurchatkin I'm not sure whether we should include the functionality or not, but I'm gonna throw in my own module (https://github.com/cjihrig/userinfo) for consideration. It works at the C level and doesn't rely on environment variables.

@rvagg yes. Seems like something that could live outside of core.

@a8m
Copy link

a8m commented Jan 31, 2015

@rvagg I agree with you, but there's a reason why.
after issue-394 opened, come three different implementations: 434, 442, 596, but we're not sure what is the right way(I'm not tagging those issues because it's irrelevant here).
thanks for @vkurchatkin CR I think 596 has the best implementation(I'm trying to stay objective).

@cjihrig I'm not sure how this could live outside of core.

@vkurchatkin
Copy link
Contributor Author

@cjihrig repl history definitely can't live outside of core

@cjihrig
Copy link
Contributor

cjihrig commented Jan 31, 2015

I haven't dug deeply into this, but just based on a little Googling, it does seem possible:

@vkurchatkin
Copy link
Contributor Author

@silverwind
Copy link
Contributor

Would love to see this in core (along with path.resolve("~") support, but that's another story).

If the C functions in @cjihrig's module work universally, I'd say that would be the best approach.

@rvagg
Copy link
Member

rvagg commented Jan 31, 2015

As I mentioned before, there are existing packages that will give you fancy repl functionality outside if core, including history but also lots of other features. replpad by @thlorenz is just one example.

My contention here is that the amount of additional code burden to get this feature in to core may suggest that it probably doesn't belong here. Core should remain lean and the node philosophy has evolved strongly into "small-core, vibrant-userland" so we need to always keep that in mind when assessing what pet-feature someone wants to bundle into core.

@thlorenz
Copy link
Contributor

Actually history is that one feature I didn't get to yet, but am very open to have this added.
Please submit PRs here :P.

@rvagg
Copy link
Member

rvagg commented Feb 1, 2015

On the topic of user home detection in core I'm a strong -1 at this stage. It's not a clean operation and exposing it to users as if it were is fraught with danger.

Consider how the suggested user-home module works for instance. If $HOME is not set on a non-Windows, non-Mac platform (i.e. the most common platforms for Node deployment: Linux and other unixes) then it assumes /home/user, which is absolutely not safe even if the common case. There is a strong case that if you cannot safely detect user home then you should fail and report. For example, an application started from Upstart or systemd where $HOME is optional, NY preference would be to fail if we really need to access user home and haven't been told where it is. A transparent operation from core conveys the impression that it's safe and can be trusted, best to encourage manual detection or using a userland module which comes with all the standard caveats that userland modules come with.

@brendanashworth brendanashworth added the os Issues and PRs related to the os subsystem. label Feb 1, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Feb 3, 2015

I'm also -1 on any type of detection involving environment variables.

@silverwind
Copy link
Contributor

@cjihrig do you know if getlogin_r() + getpwnam() works on all Unix-based OS?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 3, 2015

I'm not comfortable saying all. I've tested on OS X and Ubuntu.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os Issues and PRs related to the os subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants