Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

npm install should recursively check/install dependencies #1341

Closed
polotek opened this issue Sep 1, 2011 · 55 comments
Closed

npm install should recursively check/install dependencies #1341

polotek opened this issue Sep 1, 2011 · 55 comments
Milestone

Comments

@polotek
Copy link

polotek commented Sep 1, 2011

Currently doing an npm install will read the top level package.json and resolve dependencies. If those dependencies have sub-deps defined, it'll grab those too. However, if a top level dependency is already present but it's sub-dependencies are not, npm install does not recurse and fetch these.

This comes up in the specific case where we have a project, P, with a package.json and dependency D. Instead of managing all dependencies through npm, we want to bundle D along with it because we have a fork and it's easier for us to maintain that way. However we don't want to include Ds dependencies. We want those to be updated via npm when we deploy.

npm update works this way right now.

@polotek
Copy link
Author

polotek commented Mar 5, 2012

This issue is labeled "isaac says yes". That means he'll get to it if he has time. But if someone sent a reasonable pull request, he would take it. Taking a look at how the update command works, it shouldn't be too bad. This isn't high on my list, but would be an awesome commit to npm for someone else.

@polotek polotek closed this as completed Mar 5, 2012
@polotek polotek reopened this Mar 5, 2012
@polotek
Copy link
Author

polotek commented Mar 5, 2012

Mistaken close. Sorry.

@jwarkentin
Copy link

Holy Crap +1! I was going crazy trying to figure out why my app was crashing until I realized the dependencies were missing because of this. After that I just had to temporarily move the dependencies from the node_modules directory and reinstall, then move them back and it worked fine. Thanks @polotek for mentioning the thing about top level dependencies - that was the clue I needed. This really needs to be fixed!

@jwarkentin
Copy link

Workaround? Hah! Well, this is the best I've got. From your project directory:

mv node_modules node_modules.bak
npm install [module]
mv node_modules/[module] node_modules.bak
rmdir node_modules
mv node_modules.bak node_modules

@isaacs
Copy link
Contributor

isaacs commented Apr 23, 2013

@jwarkentin You don't need to move any dirs around. Just do npm install module and it'll clobber whatever's already there.

@arboleya
Copy link

@isaacs Sure thing.

However, my case is very particular. I have a postinstall script that compiles my coffee sources to JS, and this script looks for the coffee bin inside de node_modules folder.

I will probably: 1) walk the dirs up until I find the coffee bin or 2) automate the postinstall script to kinda force-install the right version of coffee-script (according the package.json).

@isaacs
Copy link
Contributor

isaacs commented Apr 23, 2013

Your post-install script should not look in any bin folders. Just call coffee. It'll be there. I promise.

qv: https://github.com/isaacs/npm/blob/master/lib/utils/lifecycle.js#L79-L83

@jwarkentin
Copy link

@isaacs The problem is that if you have module-a which depends on module-b and you've already installed module-b at the top level, it will never install module-b within the directory structure of module-a because it was already installed at the top level and it considers that to fill module-a's dependency. The only guaranteed way to make sure you get a clean install with all dependencies where they need to be is to install each module by itself with a clean node_modules directory.

A great way to see the problem for yourself is to do this:

npm install redis@0.7.3
npm install socket.io

socket.io depends on redis@0.7.3 but it will never be installed because it's already been installed as a top level dependency. If you reverse the order and install socket.io first then redis@0.7.3 it will work fine. As I mentioned in my comment a couple months ago, this caused my app to crash and was tricky to track down. I wouldn't have known what was going on without this bug report.

@arboleya
Copy link

@isaacs I love promisses! ❤️

I don't know since when and exactly why I was used to refer to the local bins this way, if I'm not mistaken this didn't use to work before, whatever.. it just worked, that's what matters.

Now it makes me think whether this issue is a bug or if this is the expected behavior.

@arboleya
Copy link

@isaacs Okay, the bin will be there when the script is run by npm itself.

package.json

  "scripts": {
    "test": "make test",
    "postinstall": "make postinstall"
  }

makefile

postinstall: build

build:
    coffee -cmo lib src

However, when I call make build manually from the terminal it wont work.

I guess the trick here is to have this working both ways, given this 'environmental' thing.

@isaacs
Copy link
Contributor

isaacs commented Apr 23, 2013

a) That should be a prepublish script, not a postinstall script. Don't force coffeescript on your users. Publish JavaScript, as the gods intended.
b) Why use make at all in this case? It's not as if you're actually taking advantage of the benefits of partial building. Just do npm run prepublish (or npm install) once you've got "scripts":{"prepublish":"coffee -cmo lib src"} in package.json

@arboleya
Copy link

a) You're absolutely right, but if I want to call my makefile target it'll fail as well, even as a prepublish script.

b) It's just a habit of conglomerating many kind of taks in one single place, be it some makefile, for easy maintenance and accessibility. The build target could be used from others, not just the postinstall.

This example is very optimistic, some cases are more complicated, and I'd like to maintain the 'logic' always as much centralized as possible without duplicating lines. But it's just an organizational taste, a little DRY tough.

a little more on makefiles

The trick here is to overrite the CS var passing it as argument CS=bla inside the postinstall target.

CS=node_modules/coffee-script/bin/coffee

# target used only by NPM! won't work if called manually or from other target!
postinstall:
    make build CS=coffee

build:
    $(CS) -mco lib src

Thanks @isaacs.

@isaacs
Copy link
Contributor

isaacs commented Apr 23, 2013

I guess you don't need your module building on Windows.

@arboleya
Copy link

Well pointed. But as a prepublish script it may be acceptable.

You'll must to have a terminal emulator with make, yes. But it's all there.

For development (pre-publishing) you probably already have it. (I guess)

@clayzermk1
Copy link

I agree there is a problem here.

Not recursively adding dependencies certainly keeps the size of node_modules down. What about circular dependencies? You require A that requires B that requires C that requires B... That would really make a nasty tree.

I can't help but wonder if there is another way around this. require.resolve() gets you the path to the module itself. Maybe something to help in the global namespace? I dunno, I'm just thinking here.

Am I totally off the mark?

@jfroom
Copy link

jfroom commented Nov 7, 2013

This appears to still be an open issue. Perhaps a bounty would help? I can throw a few bucks at it to get the ball rolling, but am new at this and don't want to be the first if it's bad form. Thoughts?
https://www.bountysource.com/issues/238151-npm-install-should-recursively-check-install-dependencies

@luk-
Copy link
Contributor

luk- commented Nov 7, 2013

It's not bad form. I don't really have the time to do this right now but I
bet someone will fix it pretty quickly if there's $1000 on the line.

On Thursday, November 7, 2013, Jeff Froom wrote:

This appears to still be an open issue. Perhaps a bounty would help? I can
throw a few bucks at it to get the ball rolling, but am new at this and
don't want to be the first if it's bad form. Thoughts?

https://www.bountysource.com/issues/238151-npm-install-should-recursively-check-install-dependencies


Reply to this email directly or view it on GitHubhttps://github.com/isaacs/npm/issues/1341#issuecomment-28010892
.

@iarna
Copy link
Contributor

iarna commented Dec 12, 2014

@JamesMGreene The multi-stage installer will fix this, but the changes for this specific issue aren't related to that.

@iarna
Copy link
Contributor

iarna commented Dec 12, 2014

This is going to be implemented under #6913. As such, I'm going to close this ticket and any further discussion should occur over there.

@trusktr
Copy link

trusktr commented Feb 2, 2015

@iarna I'm not sure if I'm having the same problem, but basically If I develop package B which has dependency C who's bin is called in a script called by postinstall or prepublish of package B, then running npm install in the B folder works, and the postinstall or prepublish scripts both work. npm link works too.

But now if I'm developing package A which depends on package B,but also depends on package C and has the same postinstall and prepublish scripts as package B, then package A's scripts all fail, unable to find the required C executable. If I remove package B from the deps of package A so that A depends only on package C, then the postinstall and prepublish scripts of package A work perfectly and the C executable is found (but the package is now useless without it's B dependency)!

@trusktr
Copy link

trusktr commented Feb 2, 2015

Furthermore, the prepublish scripts, which @isaacs recommends and who's executable should just be present doesn't work, command not found, and npm publish fails. I can get it to succeed if I run npm install; npm publish instead. Is that the expected usage? Are we required to run npm install before npm publish?

@trusktr
Copy link

trusktr commented Feb 2, 2015

Wait, @isaacs, you recommend only having pre/post install scripts, as per #3059 (comment), right?

@iarna
Copy link
Contributor

iarna commented Feb 2, 2015

@trusktr If I'm understanding your problem correctly, yes, your problem should be resolved by the multi-stage installer project (to be npm v3).

It's normal to have to have run npm install at some point before npm publish. You need dev deps to do dev tasks like publish and test.

@trusktr
Copy link

trusktr commented Feb 2, 2015

@iarna Sweet, looking forward to it. Thanks!

@iarna
Copy link
Contributor

iarna commented Feb 2, 2015

@trusktr Regarding 3059: In the long run, it might be desirable to remove install related lifecycles entirely. But that's not currently scheduled. There has also been some discussion about making it so that npm install (without a package name) doesn't run the prepublish lifecycle, as people find this confusing. But that's not scheduled at this time either.

@iarna
Copy link
Contributor

iarna commented Feb 2, 2015

As Isaac says in the linked ticket: Making breaking changes that break how published packages work is something that requires the utmost care. If you ask me, I doubt that non-backwards compatible packages will be a thing. Similarly, non-forward compatible changes cause enough chaos to be avoided when at all possible (see: The rollout of the ^ version prefix).

@trusktr
Copy link

trusktr commented Feb 2, 2015

@iarna Yeah, that is true. The npm command should definitely output a very visible message notifying people of the change.

Do you mind running npm install infamous@0.0.35 (I just published it) and seeing if you get 6to5: command not found and if that happens to actually be the problem of this issue?

@iarna
Copy link
Contributor

iarna commented Feb 2, 2015

Yes, that is exactly the scenario it fixes.

@trusktr
Copy link

trusktr commented Feb 2, 2015

Great! Any suggestion on how I can work around this for the time being?

@iarna
Copy link
Contributor

iarna commented Feb 2, 2015

Even with the new version we still strongly recommend you use the prepublish step for compiling, not the install steps. As I mentioned, long term we'd like to see them go away entirely.

If your concern is with having the compile steps run when you run npm install without any args, then you might try using https://www.npmjs.com/package/in-publish which lets you ONLY run the prepublish when you run npm publish.

If your concern is cruft in your directories, I recommend a postpublish script to clean up the things generated by prepublish.

@trusktr
Copy link

trusktr commented Feb 2, 2015

@iarna My concern is that the non-ES6 version of my lib should be available for people to use as in require('infamous/Molecule'), while I develop in ES6 on my side, but I also want to leave the src folder intact for people that have ES6 environments and would like to just use the ES6 code. For example, with SystemJS or jspm they can do import Molecule from 'infamous/Molecule' to import the ES6 libs directly.

I originally thought that a prepublish script would be perfect, so npm would compile things into the root of the package while leaving all the ES6 in the src folder, but then I realized that it's also happening after npm install infamous on the user side, and plus now with the bug of this github issue prepublish fails just like postinstall does with "6to5: command not found", so even if the prepublish works on my end, users will still fail to install infamous.

I was thinking about just including some steps for browserify and webpack in the readme so I could just ship ES6 code and users could have it working in those workflows, but it won't be portable to other workflows.

I think the option for me right now might be to make a publish script (or similarly named) that pre-builds everything, then finally runs npm publish, but I know, i just know, that I'm going to slip and accidentally run npm publish and publish the package in a state I didn't intend. :)

Regarding entirely removing pre/post sripts in the future, I think it makes sense to remove preinstall and postinstall, but prepublish and postpublish would still be super useful (assuming prepublish doesn't run after npm install <noargs>).

@trusktr
Copy link

trusktr commented Feb 2, 2015

@iarna Is there a timeline for npm v3?

@iarna
Copy link
Contributor

iarna commented Feb 2, 2015

@trusktr I still don't see why prepublish doesn't work for you? It's exactly like running a command and then running publish.

then I realized that it's also happening after npm install infamous on the user side

Prepublish is not called then. If someone types in npm install infamous it will not run prepublish.

Edit: I see your comment on the other ticket now, so we're on the same page now. 😊

@iarna
Copy link
Contributor

iarna commented Feb 2, 2015

I'm not willing to publically give an eta besides soon. But I did install your module flawlessly with it. 😉

@trusktr
Copy link

trusktr commented Feb 2, 2015

@iarna hehe. Sweeeeet. Well I ended up just switching to prepublish to build everything in dev before publishing, for now at least. Thanks!!

@iarna
Copy link
Contributor

iarna commented Feb 2, 2015

Awesome! I think prepublish is a much better solution anyway. 😊 Clearly we have some work to do clarifying it's use, because I've seen many people shying away from it probably for the same reasons you did.

creatorrr added a commit to creatorrr/cafe-browser that referenced this issue Apr 4, 2015
Signed-off-by: Diwank Singh <diwank.singh@gmail.com>
@Ayms
Copy link

Ayms commented Apr 9, 2015

Not sure to understand the conclusion of this long thread and the fix.

Not being really used to npm, I decided to package https://github.com/Ayms/torrent-live, then logically clean the code (as it is now) and remove the code of all dependencies related to already existing modules that I did not modify, keeping in node_modules the existing modules that I did modify only (having themselves dependenciies to non modified existing modules referenced in the package.json files):

A --- node_modules
-------------------- modified module1
-------------------- package.json
----------------------------------- node_modules
---------------------------------------------- modified module
---------------------------------------------- package.json
--------------------- modified module2
--------------------- package.json
etc...

The basic thinking was that npm publish would parse recursively the node_modules directories, include the modified modules and their dependencies.

npm install before npm publish does indeed parse correctly and add all the dependencies to the rep, which is the contrary of the initial intent since it is to keep only modified modules.

Maybe I am not using it correctly, how I am supposed to handle this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests