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.

Sign up for GitHub

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

Jump to bottom

Replace user-home with os.homedir. #2635

Merged
merged 3 commits into from
Feb 8, 2017
Merged

Replace user-home with os.homedir. #2635

merged 3 commits into from
Feb 8, 2017

Conversation

wtgtybhertgeghgtwtg
Copy link
Contributor

Summary
Replaces user-home with os.homedir. Proposed to close #2596.

Test plan
Refactors internals. Nothing should change.

@bestander
Copy link
Member

I think user-home wraps os.homedir and has some important edge case handling.
@Daniel15 has some background on it IIRC

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

wtgtybhertgeghgtwtg commented Feb 6, 2017 •
edited
Loading

user-home is just

'use strict';
module.exports = require('os-homedir')();

So the only edge case would be node<4, since it uses a ponyfill for os.homedir.

@bestander
Copy link
Member

Lol, indeed it is https://github.com/sindresorhus/user-home/blob/master/index.js.

bestander
bestander requested changes Feb 6, 2017
View reviewed changes
Copy link
Member

@bestander bestander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into that.
Just a minor refactoring please.

src/registries/npm-registry.js Outdated
@@ -12,7 +12,7 @@ import {addSuffix, removePrefix} from '../util/misc';
import isRequestToRegistry from './is-request-to-registry.js';

const defaults = require('defaults');
const userHome = require('user-home');
const userHome = require('os').homedir();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that constants.js has custom edge case handling to homedir, can you extract that part into an internal module in src/util?
I think that edge case handling may cause some issues

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like me to use that internal module for all uses of os.homedir? What would you like me to name that module?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we should use one homedir definition everywhere.
Use your best judgement and lowercase letters, something with words user, home and dir maybe :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being up at 4 AM to fix PR's is the highlight of my day, so I wouldn't bank on my best judgement.
It needs access to isRootUser (which is exported) and getUid (which is not). Would you like me to make getUid an export of src/constants.js, or move both to src/util?

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

Tested failing because babel-loader deleted the branch that was being checked.
Why does it check a branch of babel-loader, anyway?

@bestander
Copy link
Member

Thanks, @wtgtybhertgeghgtwtg.
I'll fix the test now.
We just needed a package to test that resolver works for this type of dependency.

@bestander
Copy link
Member

This should be safe to merge, all the other tests pass.

@bestander bestander merged commit 32b6260 into yarnpkg:master Feb 8, 2017
@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg deleted the drop-user-home branch February 9, 2017 03:40
mnsn pushed a commit to mnsn/yarn that referenced this pull request Feb 15, 2017
@wtgtybhertgeghgtwtg @mnsn
Replace user-home with os.homedir. (yarnpkg#2635)
bf30196 
* Replace `user-home` with `os.homedir`.

* Replace `os.homedir` with `user-home-dir`.

* Fix use `require('user-home-dir').default`.
mnsn pushed a commit to mnsn/yarn that referenced this pull request Feb 18, 2017
@wtgtybhertgeghgtwtg @mnsn
Replace user-home with os.homedir. (yarnpkg#2635)
2f0acd9 
* Replace `user-home` with `os.homedir`.

* Replace `os.homedir` with `user-home-dir`.

* Fix use `require('user-home-dir').default`.
mnsn pushed a commit to mnsn/yarn that referenced this pull request Feb 18, 2017
@wtgtybhertgeghgtwtg @mnsn
Replace user-home with os.homedir. (yarnpkg#2635)
e2c0aa5 
* Replace `user-home` with `os.homedir`.

* Replace `os.homedir` with `user-home-dir`.

* Fix use `require('user-home-dir').default`.
mnsn added a commit to mnsn/yarn that referenced this pull request Feb 18, 2017
@mnsn
# This is a combination of 8 commits.
d47b926 
# This is the 1st commit message:
add warning from issue yarnpkg#2495

# This is the commit message yarnpkg#2:

fix comments of Deniel15

# This is the commit message yarnpkg#3:

remove spaces

# This is the commit message yarnpkg#4:

fix spaces

# This is the commit message yarnpkg#5:

Drop `diff`. (yarnpkg#2592)

# This is the commit message yarnpkg#6:

Remove empty lines

# This is the commit message yarnpkg#7:

Replace `user-home` with `os.homedir`. (yarnpkg#2635)

* Replace `user-home` with `os.homedir`.

* Replace `os.homedir` with `user-home-dir`.

* Fix use `require('user-home-dir').default`.

# This is the commit message yarnpkg#8:

add space that were removed
mnsn added a commit to mnsn/yarn that referenced this pull request Feb 18, 2017
@mnsn
add warning from issue yarnpkg#2495
4fc133c 
fix comments of Deniel15

remove spaces

fix spaces

Drop `diff`. (yarnpkg#2592)

Remove empty lines

Replace `user-home` with `os.homedir`. (yarnpkg#2635)

* Replace `user-home` with `os.homedir`.

* Replace `os.homedir` with `user-home-dir`.

* Fix use `require('user-home-dir').default`.

add space that were removed

marge with master
mnsn added a commit to mnsn/yarn that referenced this pull request Feb 18, 2017
@mnsn
# This is a combination of 8 commits.
10efa97 
# This is the 1st commit message:
add warning from issue yarnpkg#2495

# This is the commit message yarnpkg#2:

fix comments of Deniel15

# This is the commit message yarnpkg#3:

remove spaces

# This is the commit message yarnpkg#4:

fix spaces

# This is the commit message yarnpkg#5:

Drop `diff`. (yarnpkg#2592)

# This is the commit message yarnpkg#6:

Remove empty lines

# This is the commit message yarnpkg#7:

Replace `user-home` with `os.homedir`. (yarnpkg#2635)

* Replace `user-home` with `os.homedir`.

* Replace `os.homedir` with `user-home-dir`.

* Fix use `require('user-home-dir').default`.

# This is the commit message yarnpkg#8:

add space that were removed
mnsn added a commit to mnsn/yarn that referenced this pull request Feb 18, 2017
@mnsn
add warning from issue yarnpkg#2495
656f5d8 
add warning from issue yarnpkg#2495

fix comments of Deniel15

remove spaces

fix spaces

Drop `diff`. (yarnpkg#2592)

Remove empty lines

Replace `user-home` with `os.homedir`. (yarnpkg#2635)

* Replace `user-home` with `os.homedir`.

* Replace `os.homedir` with `user-home-dir`.

* Fix use `require('user-home-dir').default`.

add space that were removed
connesc
connesc reviewed Mar 4, 2017
View reviewed changes
src/util/user-home-dir.js
@@ -0,0 +1,9 @@
/* @flow */
const path = require('path');
const {ROOT_USER} = require('../constants');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ROOT_USER is undefined at this point due to the cyclic dependency.

Copy link
Member

@Daniel15 Daniel15 Mar 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, that may explain #2807.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@connesc would you send a PR then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure: #2855 😉

@Daniel15 Daniel15 mentioned this pull request Mar 4, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bestander bestander bestander requested changes

@connesc connesc connesc left review comments

@Daniel15 Daniel15 Daniel15 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider replacing user-home with os.homedir.
4 participants
@wtgtybhertgeghgtwtg @bestander @Daniel15 @connesc