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

Add ability to define plugin npm dependencies #34

Closed
Marak opened this issue Aug 18, 2012 · 7 comments
Closed

Add ability to define plugin npm dependencies #34

Marak opened this issue Aug 18, 2012 · 7 comments

Comments

@Marak
Copy link

Marak commented Aug 18, 2012

I've been running into a common idiosyncrasy with plugins that require dependencies. Looks like a chance to improve the API / set a best practice.

Take for instance, a jade plugin:

/**
 * Jade broadway plugin.
 */

var jade;

exports.attach = function (options) {
  this.jade = {
    render: function (view, data, cb) {
      var html;
      try {
        html = jade.compile(view.template, options)(data);
      } catch (err) {
        if (cb) { return cb(err); }
        throw err;
      }
      if (cb) { return cb(null, html); }
      return html;
    }
  }
};

// `exports.init` gets called by broadway on `app.init`.
exports.init = function (done) {
  try {
    jade = require('jade');
    done();
  } catch (err) {
    done(err);
  }
};

Notice App.init contains a try / catch block around the require statement to determine if jade is available.

It seems this is going to be a common problem ( plugins that require dependencies ).

A good potential solution would be being able to specify npm dependencies on a per plugin basis.

// `exports.dependences` contains hash of npm deps for plugin
exports.dependences = {
  jade: "*"
}

Then, broadway and flatiron would intelligently be able to detect / load modules based on this new information.

@eschmitt
Copy link

The dust template engine seems to present an interesting case as well...

// `exports.init` gets called by broadway on `app.init`.
exports.init = function (done) {
  try {
    dust = require('dust');
    done();
  } catch (err) {
    try {
      dust = require('dustjs-linkedin');
      done();
    } catch (err) {
      done(err);
    }
  }
};

Might there be a way to handle a sequence of fallbacks in case one or more dependencies fail?

@Marak
Copy link
Author

Marak commented Aug 18, 2012

@eschmitt - Exactly. We don't want to be reimplementing try / catch / require / async logic in every plugin.

Unless there is another solution for doing this that I'm not aware of, we are going to move forward on the convention of exports.dependencies ( which follows the npm standard ).

@DamonOehlman
Copy link

@Marak I'm not sure whether it's of interest or not, but I've written a small library (squirrel) to handle this for genuinely optional modules that a package that I have may require.

The reason I wrote it in the first place was to facilitate using things like coffee-script, stylus, etc in a small build helper tool (rigger) that I wrote without having to specify them as initial dependencies that would be downloaded when the library was first installed. Plugin dependency versions are specified in the package.json file as pluginDependencies, e.g.:

https://github.com/buildjs/rigger/blob/master/package.json#L23

That section in the package.json is optional though as it is just as valid to ask squirrel to get you a specific version when you execute it:

var squirrel = require('squirrel');

squirrel('[email protected]', { allowInstall: true }, function(err, jade) {
});

Regardless of whether you think squirrel is a good fit for your needs here, I'd be very interested in your thoughts on it. Feel free to raise issues against it with issues or thoughts :)

@Marak
Copy link
Author

Marak commented Aug 28, 2012

A package may have multiple optional plugins, each which requires it's own set of dependencies. Plugins also might require that you ensure a feature set of the plugin before it can be used.

This level of granularity necessitates configuration of feature sets and dependencies be available on a per plugin basis, and not on a per package basis.

I almost just want to create a package.json file for every broadway plugin, but that seems a bit over-kill. Adding a simple and optional dependencies hash on plugins should solve this.

Will try to get a working solution for this in the next week or so.

Marak added a commit that referenced this issue Aug 28, 2012
…a plugin expecting a npm dep hash. If deps are required but not availabile, Broadway will throw first before the plugin loads. #34
@Marak
Copy link
Author

Marak commented Aug 28, 2012

So that works as expected and pretty much fixes the issue.

I'm going to integrate some friendly CLI messages to inform the user of what happened.

If we package npm with Broadway, it will be possible to install missing deps on the fly from CLI menu. I really like this idea, but the bloat of adding npm is not really acceptable. Maybe instead of require(npm), we could just cheat and shell out.

Going to think about this for a bit.

@3rd-Eden
Copy link

Have you considered of lazy installing the modules? I ran in simular issues while working on my front-end build system that has support for a lot of different pre-processors such as stylus, less, sass, coffeescript etc.. So I created a small npm wrapper that installs packages on the fly if they are not found:

https://github.com/3rd-Eden/canihaz
https://github.com/observing/square/blob/master/package.json#L45-59

Implementation example: https://github.com/observing/square/blob/master/plugins/obfuscation.js#L27-34

edit
Oh I just noticed that this was already mentioned above, ignore me ;)

@Marak
Copy link
Author

Marak commented Aug 28, 2012

@3rd-Eden - Excellant point.

If we are going to keep Broadway isomorphic, lazy-loaded modules are pretty important ( particularly when lazy-loading javascript assets in the browser ).

I'll change the require statement to a lazy getter, and place the friendly error logic inside there.

This way, in the future, we could implement an async browser script loader using the lazy-getter.

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

4 participants