0

I have two classes, exported in single instance pattern

// classA.js
let ClassB = require('./classB')
let ClassA = class ClassA {

  constructor () {
    this.name = 'ClassA'
  }

  getName () {
    return this.name
  }

  getOtherName () {
    return ClassB.getName()
  }

}

module.exports = (function() {
  return new ClassA()
})()


// classB.js
let ClassA = require('./classA')
let ClassB = class ClassB {

  constructor () {
    this.name = 'ClassB'
  }

  getName () {
    return this.name
  }


  getOtherName () {
    return ClassA.getName()
  }

}

module.exports = (function() {
  return new ClassB()
})()

In both instances, running getOtherName() will throw an error, as the other class object is an empty object.

I've read this post, which has some information for solving this with pre-ES6 class definitions (prototype style), however none of those solutions seem applicable.

For example, you can't export the class before defining the methods. I can think of ways to solve it where the classes aren't singletons, but requiring a single instance of each class makes it harder I think.

What's the best solution for this?

Benedict Lewis
  • 2,733
  • 7
  • 37
  • 78
  • The answers of your linked question explain how to solve that. You have to move those `require` statements behind the `module.exports` assignment. But such circular dependencies indicate a problem with the design of your code. – t.niese Mar 15 '18 at 16:52
  • @t.niese One module handles pubsub messages between servers, the other handles websocket connections to that server. If the app needs to send a websocket message to a user, and the user is not connected to that server, the websocket module must sent it via pubsub to the other servers to attempt delivery. Likewise, when a message is delivered via the pubsub module, it must sent to the websocket server – Benedict Lewis Mar 15 '18 at 17:00
  • Both modules depend on each other, but serve different purposes so it makes no sense to combine then. I can't think of a way to avoid the cyclical dependancy – Benedict Lewis Mar 15 '18 at 17:00
  • 1
    Singleton classes are an antipattern in Node. A module is already a unique object instance. Don't carry over patterns from other languages when you don't need them. – loganfsmyth Mar 15 '18 at 17:09
  • You say they serve different purposes, but on the other hand you say that they both can do the same because they can use each other as fallback. So it seems that you mix the transportlayer with the protocol. _pubsub_ and _websocket_ look like _deliveradapters_ and that you might want to use _communication_ class that provides methods for pub/sub and direct messaging, and this class internally determins which adapter has to be used. Or split it in `pubsub-client`, `pubsub-connection`, `websocket-client` and `websocket-connection`. And each client then includes both _connections_. – t.niese Mar 15 '18 at 17:18

1 Answers1

0

Just don't load ClassA until you actually need it. Change

let ClassA = require('./classA')
// ...

getOtherName () {
  return ClassA.getName()
}

to

getOtherName () {
  return require('./classA').getName()
}

Also

module.exports = (function() {
  return new ClassA()
})()

makes no sense. Just do

module.exports = new ClassA();

Or alternatively, since these are singleton classes, just don't use singleton classes at all and make your functions part of the module.

// classB.js
let ClassA = require('./classA');

const name = 'ClassB';

exports.getName = function () {
  return name;
};

exports.getOtherName = function() {
  return ClassA.getName();
};

since there is no reason to have a separate instance object when a module.exports is already an object.

loganfsmyth
  • 156,129
  • 30
  • 331
  • 251