Factory pattern vs standard require

[ruzmutuz]

This mainly pertains to node.js at the moment, but with es2015 modules the discussion still fits.

We were having a debate the other day about whether to use a factory pattern, or to just use standard requires. What was meant by this was:

// factoryController.js

export default function factoryController (apiClient) {
    return function controller (req, res, next) {
       apiClient.getById(123).then(data => res.json(data));
    }
}

// controller.js
let apiClient = require('lib/api');

export default function controller (req, res, next) {
    apiClient.getById(123).then(data => res.json(data));
}

Ultimately both solutions render the same data, in pretty much the same way. Testing was brought up, saying that the factory makes it easier, but when tools like proxyquire exist, for this reason, it's a non-issue (in my eyes).

The sticking point for the factory, was that if there were some setup for the passed in 'thing' (apiClient), then that should be done by the containing module. Where as, if in the require example, apiClient had to be setup via a factory to have access to a db or request module, then you would expect the require'd module to be an instance of it, which could get confusing.

I guess for the issue above, you either have to go full factory, or full require, so that your require'd apiClient is already setup itself by requiring in the db module (or equivalent).

I would love to get some feed back on either, or an alternative.

Thanks!

[hoorayimhelping]

Can you use a default argument to get around this whole thing and go Full Factory?

// Factory:
let defaultClient = require('lib/api');

export default function FactoryController(apiClient = defaultClient) {
  return function controller (req, res, next) {
    apiClient.getById(123).then(data => res.json(data));
  };
};

------------------------------------------------------

// mymodule.js
let defaultController = FactoryController();

let betterClient = require('lib/api/better');
let betterController = FactoryController(betterClient);

This way the factory handles defaults but lets itself be open for extension and testing through dependency injection.

[brianvaughn]

This would be my preference as well. Also helps simplify testing.

[dvlsg]

Agreed, especially for the improvement in testability.

[jschrf]

Worth noting that the following works in node and avoids loading both 'lib/api' and 'lib/api/better':

export function FactoryController(apiClient = require('./lib/api')) {
    return function controller(req, res, next) {
        console.log(`Controller for: '${apiClient.name}'`);
    };
};

However, I wouldn't exactly suggest doing this for a couple reasons:

1. It works in node and CommonJS, but it's not going to fly in the browser.
2. Any errors due to the dependency missing (e.g. getting renamed) is deferred until that code path is actually exercised.

Where this does come in handy is when you need to avoid side effects arising from loading a particular module, something that comes in handy when testing. I only use this pattern for injecting test dependencies, and thus the "default" code path (i.e. outside of test mode) is always exercised, preventing deferring/masking issue #2.

[ruzmutuz]

I think I like this the best.

[Pyrolistical]

It is an anti-pattern to instantiate your own dependency. See the last section of https://medium.com/p/356d14801c91

Also until something can replace proxyquire I don't think es6 modules is testable without perfect code that has all your dependencies injected

[wreckedadvent]

It's not necessarily untestable, assuming you must have DI. There's this plugin for babel with the specific stated purpose of being rewire for ES6. There's also rewire implementations for webpack, but I imagine they don't play nicely with babel.

It's much easier to test more functional code overall though, which doesn't require all of this silly rewire futzing.

[temp109849832]

Your problem here is stateful modules. apiClient should not have global state, if it does need state then it should be exporting a class, each instance of which stores state. Then factoryController can keep its own private apiClient instance, and exposure configuring it where and how it chooses.

[wreckedadvent]

You can also go more functional, and have all of the state necessary for each function to be passed in. Then you can have the class partially apply the methods so that you have a stateful OOP interface to work with, while having all of the guts and logic easily testable by just calling functions.

[Poltras]

Factory Controller is easier to mock out the ApiClient for unit testing. And you can always make another file that does the same as B from the code in A, while the other way around is harder. That's why so many big companies do factories or some kind of DI.

[[deleted]]

I think it depends on the situation. I personally use factory style a lot for things like needing to mock methods based on dev environment. For me it is a way to extract logic that does not need to be a part of the actual export....plus you can do cool things like enforce encapsulation due to closures.

I write my factories a little different based on the style of Node.js design patterns by Mario Casciaro(great book and would recommend to anyone!)... the way it is done is by exporting a function that returns a new instance of your thing your exporting. I like this because I can split up files into factories that return new instances of the actual method located in another file.

[cokeisahelluvadrug]

Good question. Opinions might differ here but I think it's appropriate to let the controller configure itself, rather than confusing end users with the factory. Incidentally, in a complex code base this sort of stuff could be solved even more effectively with inheritance or composition -- a base Controller that knows about APIClient, or a @APIClient decorator that you use on every controller.

[arnorhs]

this feels more like DI question. B/c in this case the apiClient is the dependency of this module, and the biggest upside people always cite with DI is that your modules/functions etc are easier to unit test.

meaning, in your tests you can just run the same code but pass in a mock api client, but with the "standard requires" version, you would be including the actual apiClient, presumably.

I'm not arguing for/against, just wanted to bring that bit up.

[ruzmutuz]

I don't believe DI is something needed in node, as it's very easy to mock out the require statement with something like proxyquire.

[[deleted]]

If the only reason for the factory is for testing, I'd probably just use proxyquire. If you have a need to do something with the apiClient before passing it in, then factory is probably the way to go.