plugin modules #930

Closed
tj opened this Issue May 16, 2011 · 14 comments

Projects

None yet

4 participants

@tj
Contributor
tj commented May 16, 2011

I think it's time that we all figure out something that will work for plugin-type modules. I've seen a few different flaws that are fundamentally module issues but there might be a better solution.

Here are a few different use-cases that I've come across:

connect-redis

inherits from connect.session.Store, though currently connect-redis does not have connect as a dep since it's implied, and typically you want to extend the connect that either you are already using, or express. Without npm having a unified solution, currently it may have to be express exposing it's connect exports and doing something like:

require('connect-redis').extend(express.connect);

this would be similar for something like express-resource that extend's the prototype:

require('express-resource').extend(express);

which is ok, but I see this all going down hill really fast, sure all of mine will look similar, but it'll quickly become a mess as far as the community goes.

connect

This is definitely more of a design issue but all of my express mods that alter the protos will see this as well. Connect currently alters one node http.Server.prototype method to provide multiple Set-Cookie support, but if connect is a dep of two or more modules it patches several times causing issues. I could add a flag to ensure that it's only patched once, but with different versions this would become pretty ugly. I guess versioning with plugin-ish mods really doesn't work to well

nib

Nib extends Stylus and utilizes the AST nodes, however since instanceof is effectively broken by npm not even attempting to satisfy a dep with the same module exports I had to remove every single instanceof within Stylus. Things are fine now, but I suspect these sorts of issues will be all over the place in the future.

I have no clue what this would look like in a package.json or how that would come together but figured I would mention the issues I've seen lately, let me know if you have any ideas.

@indexzero
Contributor

plugin modules (i.e. was connect-redis)

+1

This is going to be a problem for things we're doing in haibu. Also I've been wanting to refactor out the Riak and MongoDB support in winston into plugin modules because users have complained along the lines of:

"Why is Mongo being installed when I installed winston? I don't use Mongo at all" Even though that's not entirely true, you get the idea.

@visionmedia is right that one possible solution is to create conventions around extending the modules which a given plugin is working with (e.g. the examples he presented). I also agree with him that this is a really fragile solution and that it should be possible to solve at least the 80% case using npm and some extension to our version of the package.json.

Possible Solution: Introduce an extends property in package.json which would install each package as a sibling dependency and then symlink it into the node_modules directories of each of the modules with the appropriate extends:

  {
    "name": "connect-redis",
    "extends": {
      "connect": "1.4.x"
    }
  }

extending native prototypes (was connect)

-1

I almost never do this, and while I know why @visionmedia has to do this for multiple set-cookie support, I see this as an edge case that module / framework authors should work around. "If you're messing with native node prototypes, approach with caution" seems like a decent mantra.

instanceof is broken?

This is the first I'm hearing of this. @visionmedia or @isaacs can you shed some light on this for me? I think I half grok it, and I think this explains some bugs I was seeing in my tests after upgrading to npm 1.0. I also removed the instanceof usage and it worked like a charm.

@tj
Contributor
tj commented Jun 6, 2011

yeah I agree in general with the prototype thing, was just mentioning the case. I like "extends" :D

the issue was, an app itself will have stylus as a dep, and nib extends stylus, and requires access to the AST nodes, which as a dep would be completely different constructors. The plugin system would help solve that issue as well, but currently you would have to do something really lame like require('nib').extend(require('stylus')), which I guess is just the nature of JavaScript in some sense, but it's definitely not attractive, and I think it confuses users, they just want the plugin to work, passing express.connect to require('connect-redis').extend() makes little sense unless you explain it all the time. Either some conventions need to be adopted / documented or something more transparent via npm, not sure how I feel about the issue haha.

@bmeck
Contributor
bmeck commented Jun 8, 2011

I feel that the problem is coming from 2 different issues:

  1. Plugins need to be able to declare parent level relationships rather than child relationships that dependencies declare.
  2. Plugins need a convention for how they extend the sibling level modules they provide service to.

1 Npm needs to handle child to parent relationships

The first one really is npm's domain with the ability to specify that another module should be loaded before this one.

2 Npm needs a convention on how to extend parent modules

The second one is a bit trickier, but has several fairly battle tested solutions. Lets take nib / stylus as our example:

Dependency Injection

Common in Java / C++ / C# / ...

The consumer provides an endpoint that lets service providers register themselves against it (via an argument or via a registration method). This would end up looking like:

require("stylus").plugin(require("nib"))

Stylus would then need to follow some configuration to extend itself properly given the plugin. This may be helpful compared to other solutions in that multiple combinations of plugins could exist in a single process:

client.plugin(extension)

Unfortunately, this would also mean the plugin registration method would be individual to whatever was being extended, which could get repetitive, but would keep things explicit and help to deter people from randomly hijacking prototypes (it can still be done if exporting prototypes bound to clients, but I have seen very few plugins need to modify prototypes).

Hooking

Common in Lua / Functional languages

Hooking generally means allowing functions to execute prior or post of a function call and may have the ability to determine some changes to the return value. Security on this is a bit more lax than dependency injection, but the taint model could easily be applied to keep secure code.

prehook(stylus.render,nib)

Events

Common to Shell scripting / git

Pretty much the same as hooking, but instead of affecting the program, you can just fire up a script and determine if an action should be aborted/retried/etc. using a script.

stylus.on("prerender",nib.prerender)
stylus.on("postrender",nib.prerender)

Layers

Common to Connect

Make the user explicitly set up the plugins in a proper ordering. Similar to dependency injection, but may encourage single layer plugins rather than plugins that may have plugins (ie. for connect having a routing module that could be extended).

connect(
  router(
    httpMethods,
    httpHeaders
  )
)
connect.addRoute().get().header("keep-alive",function(value){return value===true})(function(req,res){})

Summation

Most of these do not seem mutually exclusive, and they seem to have various advantages to each. I am not sure a convention is needed as of right now on how to extend modules.

@tj
Contributor
tj commented Jun 8, 2011

Yeah, it gets super ugly with things like Stylus/Nib, you would have to first expose stylus to nib, and then register nib as a plugin itself:

var stylus = require('stylus')
  , nib = require('nib')(stylus)
  , style = stylus(str).use(nib()).render(...)

etc. it's not the end of the world, but I wouldn't call it elegant

@isaacs
Member
isaacs commented Jun 8, 2011
  1. npm shall not dictate how to write your package. So, all this talk of specific extension API stuff is a dead end. npm is going to drop files in folders, and then that's it. The api must be limited to require, and must not suck. Plugin hosts can do try { require("express-foo") } to see if the "express-foo" plugin is there.
  2. The goals of this need to be very limited. It's still not going to be possible (or at least, easy or supported) to for a child to dive into its dependency's dependencies.
  3. The ideal amount of bookkeeping to have to do is "none". Any solution that relies on a folder called "plugins" with symlinks or json that have to be read later, imposes a significant complexity penalty. Every part of the solution is a part that can break.
  4. There shall be no renaming of packages. Express doesn't get to call the "express-foo" package by the name "foo" just because it's cuter. That's needless complexity. (No one's suggested that this time around, but last time we discussed this feature, that turned out to be a sticky point that I'd rather avoid now.)

Currently, dependencies are a way of saying, "Put that in my require path." Plugins will be a way to say, "Put me in that thing's require path." Also, it'll only be optional. A "required plugin" is the same as a dependency. That is, a plugin necessitates one or more of its hosts to be present, but a host cannot necessitate a given plugin.

If none of a plugin's hosts are found at the level where it is being installed, then it will refuse to install. If it only has a single host, then that host will be added to the install list. Plugin packages are then installed into the node_modules folder of their host, rather than directly in the level where they would otherwise be installed.

This is effectively optional dependencies, specified in reverse.

package.json syntax:

{ "name": "express-foo"
, "description": "foo for express"
, "version": "1.2.5"
, "plugin": { "express": "~2.0.3" } }

If you do npm install express-foo, then you might expect to end up with this format:

(cwd)
`-- express@2.0.4
   `-- express-foo@1.2.5

However, perhaps this would be better?

(cwd)
+-- express@2.0.4
`-- express-foo@1.2.5

That is, should the plugin go inside its host as if it was a dependency, or should it go at the top level? Putting it at the top level involves less bookkeeping later, since we don't have to keep track of which packages were plugins and which were dependencies if you update express at some later time.

However, making them siblings means that you could easily update express, and then break the suitability of the plugin host. In this example, you might update express to 2.1.0, and then express-foo would no longer have a valid plugin host. This is currently a problem with using siblings to satisfy dependencies, and "update" can handle it. It doesn't seem like this would impose any additional hazard, just something to watch out for.

That's all the thoughts I've got for now. What do you guys think?

@tj
Contributor
tj commented Jun 8, 2011

personally I think it should go into express/node_modules, but I'm not sure of the other bundled-dep type logic involved, might be awkward to manage like you say. I agree as far as the js logic goes, we shouldn't try and get cute with transparently solving the issue, but it would be great if we/node could come up with a reasonable documented convention so it becomes familiar to people, similar to how I use .use() all over

@indexzero
Contributor

@isaacs +1 plugin is probably a better package.json namespace than extends anyway.

As for the child vs. sibling question, can you perhaps enlighten us on some of the deeper tendrils of the module system here?

I've been learning a bit more about it (and specifically caching) since I wrote pkginfo, and as I understand it thus far:

<

A dependency will have one instance (somewhere) below the top-level node_modules folder for each set of
dependencies (which are strict disjoint sets) that return different values for semver.satisfies().

<

That is:

(cwd)
+-- pkginfo@0.2.x
+-- module-a
`-- pkginfo@0.2.x
+-- module
`-- pkginfo@0.2.x

will actually only result in one instance of pkginfo being installed at the highest place in the hierarchy? My previous understanding was that each dependency would be ensured to be in the node_modules folder of each package which it was required by.

Armed with this new first-hand knowledge, my inclination is towards making them siblings for two reasons:

  1. The implementation side of things is easier as you mentioned
  2. It more closely represents the package.json file that a user would conceivably be using in such scenarios: e.g.
 { 
    dependencies: {
      "express": "2.0.4",
      "express-foo": "0.3.2"
    }
  }
@tj
Contributor
tj commented Jun 8, 2011

actually yeah now that I think about it, you have to specify the plugin in your app-level package.json anyways so I guess the placement could be ./node_modules/express-foo

@isaacs
Member
isaacs commented Jun 8, 2011

Ok, cool. So, then, implementation-wise, the "plugin" hash is like "dependencies", except that the packages are installed as siblings rather than children in the tree. If "express-foo" is a plugin to "express", then "express" should be able to do require("express-foo") to get it.

The thing that I'm finding a little unsatisfying about this is that there's no way for plugins to advertise or register themselves with the package that they plug into. But maybe that's just something that should be outside npm's domain?

@tj
Contributor
tj commented Jun 8, 2011

yeah tough call there, in the case of template engines like jade they should still be installable without express, and in their case it's good enough to imply that require('jade') will be called at some point.

Others like express-resource, which adds app.resource(), ideally it should "just work", via some sort of arbitrary hook perhaps, but even if it were to say call (when defined) require('express-resource').plugin(require('express'), I would have access to extend the prototype, but if you have two apps in the file and do not want it for both you it would be nicer to interface with an instance instead of the proto, so I don't see any decent way of automating that one.

Nib is a bit different, it doesn't modify anything, it just needs access to some stylus stuff, so for those kinds of plugins some kind of hook would work great.

@indexzero
Contributor

@isaac I've been been thinking about this a lot lately, and how developers of popular framework and modules can deeper integrate npm and package.json into their build process (node-pkginfo is an example of this) and where those boundaries should exist.

Here are some random thoughts:

  1. Any kind of static analysis of the npm ecosystem during an npm publish operation (such as enumerating and somehow including a list of all plugin modules) could be overly fragile, but I think it may have potential.
  2. Most file system operations (such as reading sibling modules package.json files) and all network operations (such as pinging npm for all known plugin modules) would force bigger frameworks / modules with a lot of potential plugins into some sort of async require system which could get ugly.
  3. I think @bmeck made good points about more commonly used patterns (such as IoC containers), but those can quickly get crufty and unmanageable.

I'm not sure if there is a silver bullet here for how to announce / register, but I do think that { "plugin": {} } is a good starting point.

@isaacs
Member
isaacs commented Jun 10, 2011

So, I think there's a valid use-case here, and that the "plugin" field in package.json addresses it kinda ok. Also, it seems like less than a Saturday of work and doesn't add any new folder structure requirements, so it doesn't violate the 6 month ban on architectural changes.

However, I'm still not thrilled about the lack of discoverability. What you really want is to do something like npm install express-gzip, and now all your responses get gzipped. The last time this subject came up, we'd talked about having something like:

{ "name" : "plugin-host"
, "version": "1.2.5"
, "directories": { "plugin" : "./plugins-go-here" } }

and then the plugin-host could just look at the contents of that directory, and know what plugins had been installed. That brings up some tricky issues, of course. For example, should that contain the actual package contents (in which case, it's not require-able any more, since it's not in the top-level node_modules), or a symlink (in which case, it won't work properly on windows), or just a bunch of empty files with package names (which is odd, but at least you'd be able to readdir and then know what to require().)

The solutions still feel fuzzy. Need to keep cooking the problem.

@indexzero
Contributor

Ping. Seems like this conversation has gone stale. Does everyone feel the same way about this? I've started to implement this myself by forcing users to do manual installs:

See: https://github.com/indexzero/nconf-redis

Installing nconf-redis

  $ [sudo] npm install nconf
  $ [sudo] npm install nconf-redis
@isaacs
Member
isaacs commented Sep 17, 2011

Rebranded as #1400 after extended discussions about it.

@isaacs isaacs closed this Sep 17, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment