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

Refactor internals #12

Merged
merged 4 commits into from
Jun 15, 2019
Merged

Refactor internals #12

merged 4 commits into from
Jun 15, 2019

Conversation

vladimyr
Copy link
Contributor

@vladimyr vladimyr commented May 2, 2019

This PR brings in bunch of internal changes:

  1. stop caculating defaultNpmPrefix eagerly
    That's pretty much self explanatory because we might not need it in case prefix is being overridden by env or user config.
  2. determining defaultNpmPrefix from process.execPath exclusively
    In all other cases we should rely on found config file (user/global) and value written inside.
  3. moving %APPDATA% (windows) and Homebrew related stuff to getGlobalNpmrc
    This is not determining global npm prefix but rather determining location of global npmrc config which should then be used to determine global prefix.

To summarize:

  1. if it is coming from env it is a envPrefix
  2. if it is coming from user config it is a homePrefix determined from homedir's npmrc
  3. if it is coming from global config (created during installation, like %APPDATA%/npm/etc/npmrc on Windows, or Homebrew's $(brew --prefix)/lib/node_modules/npm/npmrc) it is globalPrefix determined from globalNpmrc
  4. if everything else fails default prefix is determined according to process.execPath

vladimyr added 2 commits May 3, 2019 00:24
- determining `defaultNpmPrefix` from `process.execPath`
  exclusively
- moving %APPDATA% and Homebrew related stuff to `getGlobalNpmrc`
  that returns path to global config
- determining global prefix by reading global config
@sindresorhus
Copy link
Owner

@vladimyr Could use your thoughts in sindresorhus/resolve-global#3 (comment)

@sindresorhus
Copy link
Owner

// @TiagoDanin @boneskull Can you help review?

index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
@vladimyr
Copy link
Contributor Author

vladimyr commented May 4, 2019

@vladimyr Could use your thoughts in sindresorhus/resolve-global#3 (comment)

I'll give my best although I'm not yarn user and my daily driver is macos.

@vladimyr
Copy link
Contributor Author

vladimyr commented May 4, 2019

Btw I did some experiments and figured out that npm has following order of preference when it comes to determining prefix value:

  1. $npm_config_prefix env variable
  2. prefix key in user config (home dir)
  3. $PREFIX env variable
  4. prefix key in global config (inside installation dir)
    In next commit I'll modify code to take that into account...

@sindresorhus
Copy link
Owner

@vladimyr Do you intend to tackle #13 or should I merge this now (if it's ready?) and leave that one for someone else?

@vladimyr
Copy link
Contributor Author

@sindresorhus What I did here are mostly cosmetic changes ready for merge but it doesn't give us any benefit regarding #13 After investigating #13 I figured out that more thorough refactor is needed which I already started doing (locally) and I intend to finish this weekend. Sorry about late response, I'm not able to squeeze enough opensource time out of my schedule lately 🙃

@sindresorhus
Copy link
Owner

Ok. Great. I'll merge this then to keep your upcoming PR focused on fixing #13.

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.

3 participants