Nodebb: Provide better API for Plugins

[lo1tuma]

Status Quo / The Problem

Currently the recommended way to make use of NodeBB core functions is to use module.parent.require.
This approach has a few problems:

• Tight-coupling to the NodeBB core
  Using module.parent implies that the parent of the current module is the NodeBB core module that loads the plugin. This is not always the case. For example this makes unit testing impossible. If I require the plugin file in the respective unit test this would already fail because the parent module is the test file or the test runner instead of a NodeBB core module.
  Another example where this approach is insufficient is when the plugin consists of a lot of files.
  Given the following dependency graph of a plugin:

           +---------+library.js+--------+
           |         +----------+        |
           |                             |
           |                             |
      +----+----+                    +---+----+
      | lib/B.js|                    |lib/A.js|
      +-------+-+                    +------+-+
              |                             |
              |                             |
       +------+--+                +---------+---+
       |lib/C.js +----------------+lib/common.js|
       +---------+                +-------------+
  

  As you can see lib/common.js gets required under two different paths library.js → lib/A.js → lib/common.js and library.js → lib/B.js → lib/C.js → lib/common.js. This means loading a NodeBB core module in lib/common.js depends on where it was required. So in this case it would be either modules.parent.parent.require or modules.parent.parent.parent.require.
  This makes it extremely hard to structure the codebase of a plugin.

• Unstable API
  Requiring files from NodeBB implies to have knowledge about the NodeBB folder/file structure. This makes every file and folder in the NodeBB codebase part of the public API. So any simple refactoring in the NodeBB core like renaming a file would be a breaking change and thus would require a major version number release. Currently this is ignored most of the time and NodeBB violates semver by making such changes in minor releases.

Possible Solutions

I’ve thought a little bit about how to solve those problems and came up with two proposals. I only considered server-side only plugins, so I’m not sure what impact my proposals would have for client-side plugins.

Proposal 1: Dependency Injection

This would require the plugins to export a function instead of an object. This function would get called right after NodeBB has required the plugin. It gets called with an API object which would contain all functions and objects that are explicitly part of the public API.

Example

library.js

module.exports = function createMyPlugin(nodeBBAPI) {
    return {
        myMethod: function (postData) {
            // do somethind with postData and nodeBBAPI
            // e.g. calling nodeBBAPI.User.getUidByEmail
        }
    };
};

unit test

var createMyPlugin = require('../library.js'),
    sinon = require('sinon');

describe('myPlugin', function () {
    it('should do something with user id', function () {
        var api = { User: { getUidByEmail: sinon.stub().returns(42) } },
            plugin = createMyPlugin(api);

        // assert(plugin.myMethod({ post: 'foo' }) ...);
    });
});

Advantages

• Very easy to implement. Can be even implemented backwards compatible.
• Easy to test: the API object can be completely stubbed/mocked so you don’t have a direct dependency to NodeBB during the test-run.
• Explicit public API separated from NodeBB internals.

Proposal 2: Provide NodeBB core as a library

In this proposal I want to suggest splitting the NodeBB core from the NodeBB app where

• NodeBB core is everything that can be used programmatically
• NodeBB app is a pre-configured NodeBB core with bundled default plugins and cluster management

This would you allow to install NodeBB core as a dependency and built your own app around it. So when you have nodebb-core as a dependency you could easily require it from anywhere.

Example:

Building an app around nodebb-core:

var nodebb = require('nodebb-core'),
    express = require('express'),
    server = express();

nodebb.create(config, function (nodebbApp) {
    server.use('/board', nodebbApp);
    server.listen(myCustomPort);
});

Plugin using nodebb-core functions:

library.js

var User = require('nodebb-core/user');

module.exports = {
    myMethod: function (postData) {
        User.getUidByEmail(); // ...
        // Do something with postData
    }
};

package.json

{
    "peerDependencies": { "nodebb-core": "^1.0.0" },
    "devDependencies": { "nodebb-core": "1.3.0" }
}

unit test

var plugin = require('../library.js'),
    User = require('nodebb-core/user'),
    sinon = require('sinon');

describe('myPlugin', function () {
    it('should do something with user id', sinon.test(function () {
        this.stub(User, 'getUidByEmail').returns(42);

        // assert(plugin.myMethod({ post: 'foo' }) ...);
    }));
});

Advantages

• Apart from the problems mentioned in this issue it also might be quite interesting for developers who needs to customize a lot and don’t want to have NodeBB shipped with default plugins, build tools or cluster management.

Disadvantages

• Not so easy to implement. Probably also not backwards compatible.
• Doesn’t necessarily fix the unstable API problem mentioned above.
• Testing is possible, but not as nice as with dependency injection. Functions have to be stubbed on a global object instead of being able to inject fresh objects for every test.

[MegaGM]

Try to use require.main.require instead.
I may be wrong though, since I haven't even read the entire issue

[psychobunny]

@lo1tuma congrats for writing the most detailed issue that I've seen on this tracker!

[lo1tuma]

@MegaGM Thanks for the suggestion, but I still don’t see how this would work in unit tests. When I use mocha for example, require.main would be something like ./node_modules/mocha/bin/_mocha.

[barisusakli]

Somewhat related https://github.com/barisusakli/nodebb-plugin-dbsearch/commit/f56772c54af9a673a3f8251ab15ef1e78cef9185

This allowed me to test dbsearch plugin from the core tests.

[lo1tuma]

@barisusakli What do you mean by running it from the core tests?

[barisusakli]

dbsearch plugin comes bundled with core so when we test the search filters the search plugin gets called, but without the changes I made in the plugin it was failing because require.main was pointing to something else like you said. require.parent works because it always points to plugins.js

[lo1tuma]

@barisusakli Thanks for the explanation. So for other plugins that are not bundled with the core, or plugins which use a different test-runner than mocha still have this problem.

Before discussing further on any workarounds or specific solutions, do we all agree that the plugin API could be improved to provide better testability?

[barisusakli]

Before discussing further on any workarounds or specific solutions, do we all agree that the plugin API could be improved to provide better testability?

Yes, see https://github.com/NodeBB/NodeBB/issues/3321 as well

[MegaGM]

I'm sorry guys, especially @lo1tuma, for my quick and inconsistent reply.
After all I had to read the entire issue first, as I usually do.

@lo1tuma You've mentioned some of the most underestimated issues of NodeBB.
No semver breaking changes in API. Black magic with testing plugins... Every plugins developer has to deal with it.
Although it seems not so hard to recursively search, like require() does, then parse package.json files, to find the right one with "name": "nodebb", then to do something like require(path.join(theDirectory, 'src/boo/boo').babaz = mySuperStub, I still consider it as some kind of an adhoc black magic.
No one might be sure whether it will work tomorrow. I'm not sure even whether it works today. Just try to imagine what if your plugin is linked to nodebb/node_modules from somewhere else and if your test runner was installed with -g flag. Happy cwd/pwd guessing.

The issues are there, let's do something

[lo1tuma]

I'm sorry guys, especially @lo1tuma, for my quick and inconsistent reply.

No worries! require.main.require is already much better than module.parent.require, so thanks for that.

I still think the dependency injection method is the best way forward as it doesn’t rely on any kind of environment (e.g. being part of a nodebb installation) or any kind of I/O operations (like reading files, fetching data from databases or resolving modules). This is especially important for unit testing because you need to test the unit in isolation.

FWIW: I’ve already submitted a PR which implements the dependency injection method, see: #5298.

[pitaj]

I really like this idea. I agree that dependency injection does appear to be the best option.

I think this is a great opportunity to standardize and document the external API.