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

[Bug] preinstall script during bootstrap shouldn't set npm_config_prefix #1866

Closed
ibezkrovnyi opened this issue Jan 10, 2019 · 9 comments
Closed

Comments

@ibezkrovnyi
Copy link

We need to run npm update in some project as preinstall script during lerna bootstrap process.

Expected Behavior

npm update is run sucessfully

Current Behavior

npm update is run with npm_config_prefix set to some location which is wrong

Directory where "preinstall" script with npm update was run by lerna bootstrap is polluted with all files updated by npm update command.

Workaround that doesn't work

npm --prefix ./ update

Workaround that works

npx crossenv npm_config_prefix=\"\"&&npm update

Possible Solution

To comment line

config.prefix = dir;

Steps to Reproduce (for bugs)

  1. inside some project (let's name it project A) in any monorepo create "preinstall" script: "preinstall": "npm update"
  2. run lerna bootstrap
  3. observe polluted project A's directory

Context

Your Environment

Executable Version
lerna --version 3.10.1
npm --version 6.5.0
node --version 8.11.3
OS Version
NAME VERSION
@evocateur
Copy link
Member

This seems like the wrong place to run npm update, tbh. When the preinstall lifecycle is run, it is perfectly acceptable to have process.env.npm_config_prefix defined, otherwise it might inherit the wrong value from an enclosing script shell (such as the root).

@evocateur
Copy link
Member

Also, note that v3.10.2 removed the extra root install lifecycle execution, as it caused more trouble than it was worth. Not sure what you'd expect from an npm update other than ...updates?

@evocateur
Copy link
Member

...unless I'm completely wrong. Wouldn't be the first time this week, or even today. -_-

@evocateur
Copy link
Member

cuz like, yeah, just running npm run env | grep npm_config_prefix in the root of lerna's repo definitely doesn't set it to the CWD. so i must have misread something somewhere when i was extracting that bit.

thanks for your patience, @ibezkrovnyi

@evocateur
Copy link
Member

So, digging into it, npm_config_prefix is always set, it's just that it defaults to the global prefix (npm c get prefix), not the localPrefix (as it is referred to internally). Would that be sufficient?

@ibezkrovnyi
Copy link
Author

ibezkrovnyi commented Jan 11, 2019

This seems like the wrong place to run npm update, tbh. When the preinstall lifecycle is run, it is perfectly acceptable to have process.env.npm_config_prefix defined, otherwise it might inherit the wrong value from an enclosing script shell (such as the root).

  1. If I comment line:
    config.prefix = dir;

npm update in preinstall script works well

  1. if in some circumstances process.env.npm_config_prefix can be wrong, is it possible to set it to empty string "" instead of wrong path

  2. I think projectDir is wrong value for prefix because npm command itself is run with base directory projectDir, so current behavior is (please correct me if i'm wrong):

npm_config_prefix=packages/projectA
exec_in_folder(packages/projectA) `npm run preinstall` (which executes `npm update`)

which is equivalent of:

cd packages/projectA
npm update --prefix packages/projectA update

which is obviously wrong as we are already inside directory packages/projectA, no need for additional prefix which comes to us from npm_config_prefix env variable.

Possible solution

Is it possible to just erase this env variable? (config.prefix = "")

Update

So, digging into it, npm_config_prefix is always set, it's just that it defaults to the global prefix (npm c get prefix), not the localPrefix (as it is referred to internally). Would that be sufficient?

Thanks! Will test this later, 2 AM now :)

@evocateur
Copy link
Member

No worries, thanks. Digging through the npm source, I'm fairly confident this value always defaults to the global prefix. Gonna push a patch that omits setting our custom value, and it should probably be fine for your use case.

@evocateur
Copy link
Member

v3.10.5

@lock
Copy link

lock bot commented Apr 11, 2019

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants