Enhancement: recursively npm install on all pushed node_modules

[dondeli]

Based on #10 our new propose for a recursive npm install function.

[cfdreddbot]

Hey dondeli!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/82643910.

[jchesterpivotal]

Hi guys, this is a nice-looking patch.

Are you able to add a fixture app and test case?

An ongoing concern for us is that we could change behaviour for other applications that don't follow this pattern. And, if we take this PR, that we don't have any regressions in behaviour.

[dondeli]

@adrai can you add a fixture app and test case?

[squeedee]

Hi all.

I'm also a member of the buildpacks team and I just discussed this with Jacques. We cannot fairly expect you to create a running and passing test for this work. The test environment and suite are difficult to set up and operate. We are working on making it easier to use, but for now we would be really happy with:

1. A simple fixture app that can excercise this code.
2. A list of expectations you would have about this app to prove that it recursed on NPM install. You're welcome to put these in a spec file if you're familiar with RSpec, or just write them out.

We'll take that and formalize it as a test suite and ensure the tests are passing before committing the work to master.

Thanks again!
Rasheed Abdul-Aziz

[adrai]

I used https://github.com/adrai/cf-node-deploy-example to test it...

[lukaslehmann]

the app provided by @adrai shows you in an easy way and with minimum outside dependencies the problem which is solved by this PR.
you should check if there is a 2nd node_modules folder inside the pushed node_modules folder and then check if npm install will be executed there. That's basicly it..

[cf-buildpacks-eng]

We try to evaluate based on a successfully running app, that way we do
not have as many cf cli/app dependencies (such as adding $cf files) will
the app crash predictably without the patch? If so, that's sufficient for
testing. -- Rash
On Nov 14, 2014 12:30 PM, "lukaslehmann" notifications@github.com wrote:

the app provided by @adrai https://github.com/adrai shows you in an
easy way and with minimum outside dependencies the problem which is solved
by this PR.
you should check if there is a 2nd node_modules folder inside the pushed
node_modules folder and then check if npm install will be executed there.
That's basicly it..

—
Reply to this email directly or view it on GitHub
#11 (comment)
.

[adrai]

yes, it should crash...

[lukaslehmann]

yes it crashes, just tested it out.

[jchesterpivotal]

Thanks for that. That's all we need for now.

[adrai]

any planned date for merging this?

[squeedee]

There's a rough breakdown of our thoughts on the matching tracker story: https://www.pivotaltracker.com/n/projects/1042066/stories/82643910

The large comment by Jacques in that story are the notes from Him and I working this through.

The binary compilation step might be our biggest hiccup, it may affect existing users in fact.

Our main task on this team is to ensure that "online buildpacks" (Heroku buildpacks assume network connectivity) can also work in offline environments.

Take the Ruby buildpack for example. Making it work in a network-starved environment is not a problem. Before pushing an app, dependencies are 'vendored' (stored along with the app itself) as source dependencies, then during staging (when the internet is out, but the system is on the target host) the vendored dependencies are installed, including any compilation steps.

NPM does not offer such a mechanism (as far as we know), and this makes it hard for us to make binary dependencies work for Node.

It might not be obvious why this is an issue, but with your pull request, you are 'vendoring' dependencies inside the compile script. Compile is run during staging, which may be in an internet disconnected environment. We need to ensure that this code would run and not try to access the internet. The recursive traversal your code introduces, makes it necessary for an 'offline' application developer to install their dependencies recursively.

So to use your code for people without binary dependency requirements, we need a tool or offering that installs dependencies recursively. Unfortunately, this almost-always leads to some binary dependencies being compiled, which are then not compiled on the target host, but locally. It is a quandary, and we are pulling in some clever minds from the node community within Pivotal to get some suggestions of least-worst scenarios. I'm of the opinion we will need a way to embed dependencies recursively on the local machine, and then recompile them recursively on the target machine. This way all the required source is installed before the app reaches staging. There's a lot of work to prove this will work.

Additionally, this pull request means that we'll have modified code in our buildpack that may conflict with Heroku's. We merge from Heroku's buildpack to pick up their latest work, and we try to avoid merge conflicts and test coverage problems. Once we have a solution that works for offline users, we may want to get Heroku to accept it as a PR.

We're still actively investigating this and will respond back shortly.

[jchesterpivotal]

Hi guys;

Sorry for being quiet again for so long. We've asked another pivot to jump in to help us review this PR, we hope to get some word on it soon.

Cheers,

JC.

[cgfrost]

Hi, I'm the other Pivot. I have had a quick look over this and realised it's not simple. I should get to reviewing this tomorrow afternoon (UK time) I hope.

[cgfrost]

Apologies for taking longer to look at this than I had hoped.

I've read through all the conversations now and I think I understand the problem this pull request is meant to solve, also why it conflicts with the concept of an offline buildpack. I work a lot with the Java buildpack but I'm not familiar with the changes made to support off-line mode in the Node buildpack and I haven't figured it out in the last hour. So I'll have to defer on what effect this change would have on things still working offline.

I'm concerned that this essentially extends NPM functionality and if NPM decides to add better/different support for private or binary modules in the future then we will have diverged and could end up having to break backwards compatibility for Node apps on CF. I think this is also likely as 'node_modules' as an abstraction isn't intended to be modified outside if an NPM command.

Given your problem, which I think is fairly reasonable for a real Node project, one solution is a private NPM repository. This has already been discussed, aside from the hassle of setting one up I think we could do more to support authentication against private repos in the buildpack. Private repos are better for an entire organisation with multiple Node projects going on, it's really too much for a single project. That's an issue for another day though.

I've forked your sample app and made some changes to allow private modules to be used and automatically installed by NPM by using local file paths in the package.json file. I've also set it back to the default buildpack. Have your Grunt task copy your own modules to a new 'private_modules' directory in each app instead of the 'node_modules' directory. This approach should work in the offline case as well, once 'vendored' appropriately. It's easier to understand by looking at it, especially all the package.json files. See https://github.com/cgfrost/cf-node-deploy-example

I'm following this issue now and I'll try to respond a little quicker now I'm up to speed. Let me know if you have any issues with the approach I've suggested. I've looked at the problem of structuring larger Node projects before and there is definitely a lack of best practice out there but it must be doable considering how many big Node projects there are.

Thanks, Chris.

As a side note, please don't use 'command' in your manifest to start an application, this functionality could be removed from all CF buildpacks at some time in the future. For a Node application it's recommended to use the NPM start script, I made this change in the sample app as well.

[adrai]

The problem with this approach for us is that we use these private modules in multiple apps (for the same project) in this way we have to copy these private modules to each app... and this is not really nice when working locally... but I thank you for having investigated on this.

[adrai]

...and... does these references in the package.json really work? What happens on npm install?

[cgfrost]

Hi,

NPM works just fine, it follows the file reference and installs what it finds to the local node_modules folder. The good thing is that it also work recursively on private modules. So when own_a gets installed from the private_modules folder it's dependency on own_b is also resolved from the private_modules folder. If you download it and run npm install then look at the package.json for own_a in node_modules it will have some extra tracking information added by NPM showing where it came from.

  "_id": "own_a@1.0.0",
  "_shasum": "b461cd55551eabe167d8beaf9befa7e31d42fa33",
  "_from": "private_modules/own_a",
  "_resolved": "file:private_modules/own_a"

Sorry if I misunderstand, I believed your Grunt script is already copying these own/private modules round to all the separate apps in the project. Is this where you are using sym-links etc.., for local development. You could still use that approach for local dev, just sym-link in to the new private_modules folders instead.

Chris.

[adrai]

I will try...

so I assume an npm install will recursively npm install on own_a and in own_b

Just one more question: is the dependency in own_a of own_b correct? ./private_modules/own_b

[cgfrost]

Yes and yes.

Local dependences are always resolved against the root of the project doing the resolving, regardless of where dependence is actually located.

Chris.

[adrai]

oh no... then this is a problem... I don't know who is using own_b (i.e. a logging module) It can be referenced and installed from everywhere...
for example directly from the app or from an other module.
For example when own_a can work on its own or as dependency of something else...

[jchesterpivotal]

Hi guys,

Based on our own research and @cgfrost 's additional review, I don't think we can accept this PR. I'm sorry it took so long to settle this matter, but we were determined to ensure that this change got a full and fair review.

Cheers,

JC.

[adrai]

Is this really a practically solution?

[cgfrost]

NPM & Package.json can be very flexible but it has it's limits. Once those limits have been reached the next step up is hosting a private repository. I realise that's a pain but that's the progression you have to make.

FYI, here is the full doc for NPM local paths from npm help package.json. You may be able to get a little further with absolute file paths and a single master 'private_modules' repository. You would then have to enhance your Grunt script to manually scan and update the package.json file of every project to copy it's dependencies in before deploying to a server. It's arguable if this is more work or not than hosting your own repository as your starting to replicate that kind of functionality.

Local Paths
As of version 2.0.0 you can provide a path to a local directory that contains a package. Local paths can be saved using npm install --save, using any of these forms:

         ../foo/bar
         ~/foo/bar
         ./foo/bar
         /foo/bar

In which case they will be normalized to a relative path and added to your package.json. For example:

         {
           "name": "baz",
           "dependencies": {
             "bar": "file:../foo/bar"
           }
         }

This feature is helpful for local offline development and creating tests that require npm installing where you don't want to hit an external server, but should not be used when publishing packages to the public registry.

I'm sorry you haven't got the outcome you were looking for, I hope it doesn't hold you back too much.

Chris.

[adrai]

fyi: an other alternative could be to do this recursive stuff in a postInstall script

[adrai]

ok i tested my last approach with a preinstall script... and it works... so if everything (all 20 apps) works this way we can use the standard nodejs-buildpack