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

Dependency management #14

Open
ForbesLindesay opened this issue Aug 21, 2012 · 13 comments
Open

Dependency management #14

ForbesLindesay opened this issue Aug 21, 2012 · 13 comments

Comments

@ForbesLindesay
Copy link
Collaborator

We have a growing problem with a lot of dependencies. Most are going to be needed most of the time, but we only need coffee-script, jade, stylus, request, etc. for specific tasks. There are also loads (EJS, haml, less etc.) that we sometimes need but at the moment fore the user to install globally. I came accross a library called squirrel when checking out yaml parsers on npm. It works absolutely brilliantly and simply installs your dependencies at runtime.

As an experiment, I set up the following to compile a jade file:

var squirrel = require('squirrel');
squirrel.defaults.allowInstall = true;
squirrel.defaults.cwd = __dirname;
function autoLoad(fn, cb) {
    try {
        fn(cb);
    } catch (ex) {
        if (ex.code === 'MODULE_NOT_FOUND') {
            var match = /Cannot find module \'([^\']*)\'/g.exec(ex.message);
            var missingDependancy = match[1];
            console.log('Installing '+missingDependancy+' this may take a few secs.');
            squirrel(missingDependancy, function (err) {
                if (err) return cb(err);
                autoLoad(fn, cb);//try again
            })
        } else if (cb) {
            cb(ex);
        } else {
            throw ex;
        }
    }
}

autoLoad(function (cb) {
    require('consolidate').jade('./input.jade', {}, cb)
}, function (err, val) {
    if (err) throw err;
    else console.log(val);
});

It works because consolidate-build always throws exceptions caused by requiring uninstalled modules synchronously. This means that the above sample will run providing you have an internet connection and squirrel installed. The first time it runs it takes a little bit of time, and requires an internet connection, but after that it runs at pretty much full speed because it just requires modules in as usual.

P.S. My internet seems to be fixed so I'll talk to you tomorrow morning my time(this evening your time).

@karthikv
Copy link
Owner

Sounds like a good idea. We could get rid of coffee-script, jade, stylus, adm-zip, mime, socket.io, and uglify.js. I believe request should be moved to devDependencies as well. This'll clean up quite a bit; we'll talk about it at our meeting.

@ForbesLindesay
Copy link
Collaborator Author

Yeh, just figured I'd pop out some food for thought while I remembered. This would cut the size by about 15Mb, which is currently most of the size of the download.

@ForbesLindesay
Copy link
Collaborator Author

I tried having a go at this on a branch, but it's not passing unit tests. May be worth deferring this to the following release.

To make this work we'll need to have some way of detecting the dependencies needed for compiling a certain file format. Ideally consolidate-build would provide these in an array, and I'm happy to do that for less sass markdown etc. but I need agreement from @visionmedia because I want to match consolidate.js's API. The problem is they throw async exceptions (unless you use the render version of consolidate's API). The way the error is currently thrown, it can't be trapped, which means we can't auto install dependencies when the error is thrown.

@ForbesLindesay
Copy link
Collaborator Author

P.S. I created an issue for that error thowing problem tj/consolidate.js#59

@karthikv
Copy link
Owner

I'm having second thoughts about going through with this. It seems to be quite a bit of trouble, as we need to involve others and get modifications to external libraries. I can see why people might not want so many dependencies, but waiting 10 seconds for them to install really isn't a big burden. It also makes it difficult to run unit tests on Travis, as we'll need some way of instructing it to automatically reply yes when prompted for dependencies.

In light of that, I'm in favor of closing this entirely.

@ForbesLindesay
Copy link
Collaborator Author

The problem is it's fine as a very opinionated framework, but not when we want to keep this un-opinionated. At the moment we just install jade, coffee-script and stylus. If you want those three, great, it's installed them for you. If we install all possible template engines it won't be 10 seconds, it'll be 10-15 minutes (potentially for every re-install/update).

If we go the other route, and don't install dependencies, then we currently make the tool really hard to use for anyone not compiling jade, coffee-script and stylus.

I don't think the tests is a huge problem. We're going to end up developing our own version of squirrel I think because it relies on calling npm as a command, which could get flaky and relies un-necessarily on npm being installed.

We can install dependencies roughly as follows:

var npm = require("npm")
npm.load({}, function (err) {
  if (err) throw err;
  npm.commands.install(["dep1", "dep2"], function (err, data) {
    if (err) throw err; //can't install dependency

  })
  npm.on("log", function (message) { console.log(message); });
});

It should be fairly simple to wrap that up in such a way that by default it asks the user for confirmation, but it can be configured by the unit test runner to not ask the user for permission.

Even if we're not planning to do this automatically, we should still let the user install dependencies manually in a way other than npm install -g because I may want to install qejs for nodefront, but that doesn't mean I want it installed globally and in my path etc.

@karthikv
Copy link
Owner

Yes, but I'm not interested in developing our own version of squirrel. It seems much more logical to me to focus on other features rather than this dynamic dependency management.

I do agree with you, however, that it should be easy for the user to install dependencies manually. Perhaps something like nodefront add-template jade for the feature you describe.

As for being unopinionated, I see no problem in including a few defaults. Your comment about how this makes "the tool really hard to use for anyone not compiling jade, coffee-script, and stylus" seems like an overstatement for me; running npm install -g [template], at least for the time being, isn't a huge burden. As mentioned before, however, I would prefer the built-in template install.

I plan to release 1.0.0 before acting on this, though, since it's overdue.

@ForbesLindesay
Copy link
Collaborator Author

Getting an error that just says the module was not found makes it really hard for the user. If we made it just print You must run npm install -g [template] then terminate the program, it would at least be obvious how to go about using those other templating libraries. The code required to implement something similar to squirrel is tiny, and could live in another module which I'd happily maintain. I think we could then simply have dependencies.json or something similar which would look like:

{
  ".jade": ["jade"],
  ".less": ["less"]
}

@ForbesLindesay
Copy link
Collaborator Author

Once #36 is merged this will be a trivial change. We will just need to switch to rendering files one at a time rather than in parallel and install load-engine

and then call:

loadEngine(build[extension].engines)
  .then(function () {
    return build[extension].renderFile(filename, options);
  });

Instead of:

return build[extension].renderFile(filename, options);

@karthikv
Copy link
Owner

karthikv commented Feb 3, 2013

I'm still not convinced that this is necessary, based off my previous logic; it can break Travis, it adds extra complexity, and it doesn't really provide more convenience. Running one npm install -g [library] command is all one needs to do. Additionally, I looked over load engine, and it doesn't seem to have any tests confirming that it works.

@ForbesLindesay
Copy link
Collaborator Author

I'll add some unit tests, so far all I've done is manual testing. The problem is not that npm install -g [library] requires an additional command, the problem is:

  1. If you install a dependency like that you skip all the nice features of npm's nesting model and are forced to make that dependency globally installed.
  2. At the moment nodefront has a lot of pretty hefty dependencies as a result of the fact that you don't want to have to go through that process for libraries you frequently use.
  3. The dependencies that are bundled can't easily be updated without updating nodefront. Since it's the responsibility of transformers to maintain compatibility with the latest version of each library, nodefront shouldn't need to lock to specific versions of these things.

We could easily make this work for things like uglify-js as well and end up with an npm package that had a tiny number of dependencies and installed really quickly and just paused for a moment longer the first time you ran each command.

@ForbesLindesay
Copy link
Collaborator Author

load-engine now has very thorough unit tests and travis-ci integration enabled.

@karthikv
Copy link
Owner

karthikv commented Feb 3, 2013

All right, I'm convinced by your logic, given that you can get this to work with Travis and not make the code overly complex (judging by the simple interface you have for load engine, the latter won't be an issue).

Go ahead and dynamically load all packages you think don't need to be installed by default. I'll look over the changes once you post them.

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

No branches or pull requests

2 participants