CODEWITHSUNDEEP
javascriptnode.jswebpackes6-class
Sean
Sean

Reputation: 655

JavaScript ES6 circular require references

I've moved a bunch of data onto the UI represented by classes. The different classes all map to each other using FKs. I'm trying to keep references to those data classes to only 1, to avoid the confusing pointers, so I keep the FKs as Ints on the related classes, then look them up when I need to. Originally I had no Cache class, but I tried to create a pipe for the data between the UI and data, and funneled all the cached-data requests through its own class (which I thought would help with the circular references too) but no dice unfortunately.

I sandboxed the below example to try to simplify my issue. I used static methods to simplify the example. Also the sandbox has more-easily hierarchical classes, unlike the app. The example's probably overly-complicated but I'm trying my best to show why I (think I) need the circular references. This might be an issue with webpack, or maybe a philosophical one where I shouldn't have circular references, any feedback would be incredible! (I think the same scenario would break node.js as well)

Cache.js

const Person = require("./Person.js")
const Group = require("./Group.js")

class Cache {
    static set() {
        Person.set([
            { name: "Sean", groupID: 1 },
            { name: "Greg", groupID: 1 }
        ])
        Group.set([ {id: 1, name: "A"} ])
    }

    static getGroupByID(id) {
        return Group.getByID(id)
    }
    static getPeopleInGroup(id) {
        return Person.getInGroup(id)
    }
    static getFirstPerson() {
        return Person.getFirstPerson()
    }
}


module.exports = Cache
/* use closures?
module.exports = {
    set: () => Cache.set()
    getGroupByID: (id) => Cache.getGroupByID(id),
    getPeopleInGroup: (id) => Cache.getPeopleInGroup(id),
    getFirstPerson: () => Cache.getFirstPerson()
}
*/

Person.js

const Cache = require("./Cache.js")

const list = []

class Person {
    static set(people) {
        people.forEach( p => {
            list.push( new Person(p) )
        })
    }
    static getInGroup(id) {
        const ar = []
        list.forEach( p => {
            if (p.data.GroupID == id)
                ar.push(p)
        })
        return ar
    }
    static getFirstPerson() {
        return list[0]
    }
    constructor(data) {
        this.data = data
    }

    sayHello() {
        const group = Cache.getGroupByID(this.data.groupID)
        const groupName = group.data.name
        console.log(`${this.data.name} from ${groupName}`)
    }
}

module.exports = Person

Group.js

const Cache = require("./Cache.js")

const hash = {}
class Group {
    static set(groups) {
        groups.forEach( g => {
            hash[g.ID] = new Group(g)
        })
    }
    static getGroupByID(id) {
        return hash[id]
    }

    constructor(data) {
        this.data = data
    }
    sayPeople() {
        const ppl = Cache.getPeopleInGroup(this.data.ID)
        console.log(`${this.data.name} has ${ppl.join(", ")}`)
    }
}

module.exports = Group

app.js

const Cache = require("./Cache.js")
Cache.set()
let person = Cache.getFirstPerson()
person.sayHello()
let group = Cache.getGroupByID(person.data.groupID)
group.sayPeople()

In this example, the first bad reference is:

Class Person { ...
  sayHello() {
    const group = Cache.getGroupByID(this.data.groupID)

(Cache is an empty object)

Upvotes: 1

Views: 370

Answers (1)

jfriend00
jfriend00

Reputation: 707426

So, cache.js requires person.js and then person.js requires cache.js. That is a circular require that gives you the problem you see.

The design issue here is that person.js checks cache.js for data (that seems reasonable), but you've made methods in cache.js that require knowledge of specific data types from the cache. That creates your circular dependency.

The usual generic answer to these circular dependencies is to figure out what code is needed by both modules and break that out into it's own module where that module does not depend upon anything.

It isn't quite obvious to me how you would do that in this case. Instead, the problem seems to be caused because you made statics in the cache like getGroupByID() that have NOTHING to do with the cache at all and are creating these circular dependencies. That seems to be the core problem. If you remove those from cache.js, then that will free up cache.js to just do its job generically and you can remove the two require() statements causing the circular dependency from them.

Then, the question comes where to put those statics. These seem to be a bit of design problem. You're apparently trying to have person.js and group.js be peers that don't know anything about one another. But, you have methods where that isn't really true. sayHello() in person.js ends up calling into group.js and sayPeople() in group.js ends up calling into person.js.

I would suggest that you need to make a new common base class for group.js and person.js that contains shared implementation and perhaps that's where you can put those statics. I don't exactly understand what the underlying code is trying to do to know for sure that this is the best solution, but the idea is that you make person.js depend upon its base class rather than person.js depend upon group.js, thus breaking the circular dependencies.

Upvotes: 2

Related Questions