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] Npm installs devDependencies of url based dependency (git://) #2784

Closed
Greg-Smulko opened this issue Feb 26, 2021 · 15 comments
Closed
Assignees
Labels
Priority 1 high priority issue Release 7.x work is associated with a specific npm 7 release Wontfix this will not be worked on

Comments

@Greg-Smulko
Copy link

Greg-Smulko commented Feb 26, 2021

Linking npm/npm#21153.

Current Behavior:

devDependency of a dependency is installed when its referenced using URL (e.g. "git://github.com/company/myDependency.git#v10.17.30",

Expected Behavior:

devDependency should never be installed, regardless of the method of how the package is referenced.

Steps To Reproduce:

Create a dependency with this config:

{
  "name": "myDependency",
  "version": "10.17.30",
  "repository": {
    "type": "git",
    "url": "https://github.com/company/myDependency"
  },
  "devDependencies": {
    "node-sass": "^4.13.1"
  }
}

Use this dependency in a project:

{
  "name": "myProject",
  "dependencies": {
    "myDependency": "git://github.com/company/myDependency.git#v10.17.30"
  }
}

Run npm install on the project.

There is a failing attempt of installing node-sass ver. 4.13.1, even though it should not be.

Environment:

  • Node: v15.8.0
  • npm: 7.6.0
@Greg-Smulko Greg-Smulko added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Feb 26, 2021
@ljharb
Copy link
Contributor

ljharb commented Feb 26, 2021

Try with npm v7.6.0?

@Greg-Smulko

This comment was marked as outdated.

@dlmanning
Copy link

Same problem in v7.6.3

@yedpodtrzitko
Copy link

#492 (dependencies starting with file:) seems to be related

@nlf
Copy link
Contributor

nlf commented Apr 9, 2021

this is somewhat intentional, but there is a bug here i think.

to go into further detail, when you install a git dependency (or a directory) into your project, we assume that the target is not necessarily a fully realized package. we clone the repository, we install the dev dependencies and we run relevant lifecycle scripts (like prepare) to ensure we have as close to what the published result of that package would be as possible. we then pack that directory into a tarball and install that resulting tarball into your project.

it seems where we're going wrong is that we're failing to remove those transitive dependencies when we pack the resulting tarball. we'll look into this further, thanks for the report!

@nlf nlf added Priority 1 high priority issue and removed Needs Triage needs review for next steps labels Apr 9, 2021
@darcyclarke darcyclarke added this to the OSS - Sprint 28 milestone Apr 14, 2021
@thorn0
Copy link

thorn0 commented Apr 16, 2021

we install the dev dependencies and we run relevant lifecycle scripts (like prepare)

Does it make sense to do this when there is no such scripts in package.json?

@thorn0
Copy link

thorn0 commented Apr 16, 2021

Is there probably a way to tell npm that a package is "fully realized"?

@dlmanning
Copy link

It would be helpful to have a way to toggle this behavior. My company uses private repos to host forks of various projects. Some of those projects have native dependencies that will not build correctly on npm 7, hence this behavior is actually a blocker to our adoption of npm 7.

@RudeySH
Copy link

RudeySH commented Oct 25, 2021

I don't know about git://-based dependencies, but since npm 7.0.9, devDependencies are installed for github:-based dependencies. As of October 25th 2021, this is still happening with the latest version of npm (8.1.1).

Related: #3937

@Greg-Smulko Greg-Smulko changed the title [BUG] Npm install devDependencies of url based dependency (git://) [BUG] Npm installs devDependencies of url based dependency (git://) Oct 26, 2021
@koooge
Copy link

koooge commented Nov 1, 2021

#2610 related I guess.

@ruyadorno ruyadorno self-assigned this Feb 22, 2022
@ruyadorno
Copy link
Contributor

hi @Greg-Smulko thank you so much for taking the time to report this issue and the great description!

I'm trying to reproduce the problem described here and it jumped to my attention that what you stated as "Expected behavior":

devDependency should never be installed, regardless of the method of how the package is referenced.

is not in fact how it works, as much more thoroughly explained by @nlf in his comment:

to go into further detail, when you install a git dependency (or a directory) into your project, we assume that the target is not necessarily a fully realized package. we clone the repository, we install the dev dependencies and we run relevant lifecycle scripts (like prepare) to ensure we have as close to what the published result of that package would be as possible. we then pack that directory into a tarball and install that resulting tarball into your project.

continuing from @nlf's findings, I created a couple of minimalist test scenarios to validate that the behavior is indeed working as expected:

Scenario 1: git-based dependency has a package with a failing postinstall script

Installing a dependency test-git-repo that lists @ruyadorno/postinstall-failure as a dev dependency. This will fail as expected since test-git-repo needs to install all its dependencies in order to be prepared.

$ npm init -y
$ npm i git://github.com/ruyadorno/test-git-repo#failed-dev-dep
npm ERR! code 1
npm ERR! git dep preparation failed
npm ERR! command node $HOME/.nave/installed/16.14.0/lib/node_modules/npm/bin/npm-cli.js install --force --cache=$HOME/.npm --prefer-offline=false --prefer-online=false --offline=false --no-progress --no-save --no-audit --include=dev --include=peer --include=optional --no-package-lock-only --no-dry-run
npm ERR! npm WARN using --force Recommended protections disabled.
npm ERR! npm ERR! code 1
npm ERR! npm ERR! path $HOME/.npm/_cacache/tmp/git-clonebA75lN/node_modules/@ruyadorno/postinstall-failure
npm ERR! npm ERR! command failed
npm ERR! npm ERR! command sh -c exit 1
npm ERR!
npm ERR! npm ERR! A complete log of this run can be found in:
npm ERR! npm ERR!     $HOME/.npm/_logs/2022-02-22T22_39_56_687Z-debug-0.log

npm ERR! A complete log of this run can be found in:
npm ERR!     $HOME/.npm/_logs/2022-02-22T22_39_53_442Z-debug-0.log

Scenario 2: Removing scripts from my git-based dependency

If I take away my prepare script from my test-git-repo dependency, I can now install the dependency properly, even though it still has @ruyadorno/postinstall-failure listed as a dev dependency:

$ npm i npm i git://github.com/ruyadorno/test-git-repo#no-scripts

up to date, audited 2 packages in 2s

found 0 vulnerabilities

How can users being affected by this behavior proceed?

Given that devDependencies of git-based dependencies are in fact possibly installed, there's something that the users impacted by this behavior can do:

  1. Right now: In case you manage the installed git dependency, removing all of postinstall, build, preinstall, install, prepack and prepare scripts AND workspaces definitions will make sure to opt-out of having to install dev dependencies, since that's what the cli looks for in order to determine whether rebuilding (or realizing) the package is needed.
  2. For the future: Propose an RFC to add an option to opt-out from installing dev dependencies for git deps as proposed by @dlmanning, I can see it has the potential to raise many important questions: should we be able to opt-out per dependency? should it provide an interface for opting-out of realizing all git deps? should we also add a way to opt-in (instead of the current conditions)?

Conclusion

I'm going to close this issue now as I could not surface any hidden/extra bug in the expected behavior of git-based dependencies and their scripts. An ideal follow up here would be for some of the interested users to create an RFC for opting-out of running scripts for git-based deps so that we can discuss that alternative further.

@ruyadorno ruyadorno added Wontfix this will not be worked on and removed Bug thing that needs fixing labels Feb 22, 2022
@chevsunpower
Copy link

chevsunpower commented Apr 12, 2022

OMG I just spent three days trying to figure this out. My shop is strict. They will not give me a private npm registry which would have made this far less of a headache. Instead they just install the couple internal library packages we have as git dependencies. The docs for npm install say that as long as there is no prepare script then dev dependencies will not be installed.

One of our libraries uses node-canvas and getting that to build in all relevant spaces is just a nightmare sometimes, especially on ARM architecture with our M1 macbook pros we've been assigned. But oh well, at least we can wrestle with that in the library repo itself and then just build the dist directory (which we check-in to git since we treat the repo like a registry), right? Nope. Even though there is no prepare script and every single dependency is in devDependencies, it was STILL trying to install dev dependencies and failing to build node-canvas. But I'm not about to troubleshoot node-canvas for the 500th time when it shouldn't even need to install it!

Google search after google search after stack question after stack question and three days later I find this thread.

Right now: In case you manage the installed git dependency, removing all of postinstall, build, preinstall, install, prepack and prepare scripts AND workspaces definitions will make sure to opt-out of having to install dev dependencies, since that's what the cli looks for in order to determine whether rebuilding (or realizing) the package is needed.

Renamed the build script in the library's package.json and it finally stopped trying to install dev dependencies. What a nightmare. At least it's finally resolved...

PS - Here's some google keyword fodder for the poor lost souls still on the barge of the damned like I was 😂

Why is npm installing dev dependencies when trying to install a dependency from a git repository?
npm-cli.js install --force --prefer-offline=false --prefer-online=false --offline=false --no-progress --no-save --no-audit --include=dev --include=peer --include=optional --no-package-lock-only --no-dry-run
I don't have an npm prepare script but git repository dependency still installs dev dependencies.
M1, ARM, canvas, konva, node-canvas, node-gyp, attempting to build even as dev dependency with NODE_ENV=production
npm install, --production, --only=prod, --omit=dev

@chevsunpower
Copy link

chevsunpower commented Apr 12, 2022

For the future: Propose an RFC to add an option to opt-out from installing dev dependencies for git deps as proposed by @dlmanning, I can see it has the potential to raise many important questions: should we be able to opt-out per dependency? should it provide an interface for opting-out of realizing all git deps? should we also add a way to opt-in (instead of the current conditions)?

Honestly, just updating the documentation to include these other script steps as triggers for dev dependency installs would have saved me hours and hours of troubleshooting. I'm sure there are others out there floundering, wondering what they're doing wrong. A way to opt out would be great, but just knowing that the CLI looks for more scripts than just prepare would save anyone in mine and OP's boat a lot of headaches.

@dlmanning
Copy link

dlmanning commented Apr 19, 2022

In our case I had to eliminate the forked dependency from our build (last year) to update to npm@7 and workspaces. If this hadn't been feasible I would probably have migrated to a different package manager. Make of that what you will.

One thought regarding this:

I can see it has the potential to raise many important questions: should we be able to opt-out per dependency?

Node now has (for better or for worse) a mechanism that allows me to override dependency resolution on a per dependency basis, so I don't think it's unreasonable to provide some sort of escape hatch in the context of installation for people stuck with this. Even if I had to literally wrap the git dep in another package that uniformly specified a rule for ALL of its dependencies I would still take it.

@falahati
Copy link

falahati commented Oct 19, 2022

build is very generic and is almost always included in all packages. I suggest removing it from the list of scripts that forces the installation of dev dependencies. This makes total sense since build is not even a part of lifecycle scripts so checking it while not even running it at any point in time when installing a package makes no sense.

otherwise, if you are against this, the least we can do is to add it to the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority 1 high priority issue Release 7.x work is associated with a specific npm 7 release Wontfix this will not be worked on
Projects
None yet
Development

No branches or pull requests