Objection.js: Circular dependency issue regarding model relationships

[kouak]

As of now, when defining a relationship in a Model class, the documentation states this :

    // The related model. This can be either a Model
    // subclass constructor or an absolute file path
    // to a module that exports one. We use the file
    // path version in this example to prevent require
    // loops.

Setting a string pointing to the file where the dependant model feels more like a workaround than a proper solution.

I've seen this issue tackled in graphql-js using thunks. See here : https://github.com/graphql/graphql-js/issues/373

My proposal is to allow the developer to write this :

import RelationModel from './RelationModel';

export default class MyModel extends Model {
/* ... */
  static relationMappings = {
    relation: {
      modelClass: () => RelationModel,
    },
  };
}

[koskimas]

I'm not 100% sure about this, but I think your suggestion only works because of babel transpilation and the fact that

import RelationModel from './RelationModel'

is transpiled into

var RelationModelModule = require('./RelationModel')

and then used like this

modelClass: () => RelationModelModule.default

The RelationModel module is transpiled into something like this.

function RelationModel() {
 ...
}

module.exports.default = RelationModel;

Now in case of circular dependeny, thing work since the imported module points to the module.exports object of RelationModel.js file which remains the same only because babel doesn't assing a new value to it but adds an exports.default property instead.

Using normal requires, things would fail because

const RelationModel = require('./RelationModel');

would be an empty object (the default module.exports) and its value would not change when the row module.exports = RelationModel; is executed in the RelationModel.js file.

I don't know how ES6 modules will eventually be implemented in node.js, but this could potentially break when it happens.

Having said all this, can't you already achieve this just by saying:

import RelationModel from './RelationModel';

export default class MyModel extends Model {
/* ... */
  static get relationMappings() {
    return {
      relation: {
        modelClass: RelationModel,
      }
    };
  }
}

[koskimas]

Here's a test case to prove my point:

// MyModel.js

const RelationModel = require('./RelationModel');

class MyModel {
  static get relationMappings() {
    return {
      relation: {
        modelClass: () => RelationModel
      }
    };
  }
};

module.exports = MyModel;

// RelationModel.js

const MyModel = require('./MyModel');

class RelationModel {

};

module.exports = RelationModel;

// test.js

const RelationModel = require('./RelationModel');
const MyModel = require('./MyModel');

// This prints `false`
console.log(MyModel.relationMappings.relation.modelClass() === RelationModel);

[koskimas]

Adding what you suggested would be difficult because of the way the modelClass property validation is implemented currently. Adding support for this:

  static relationMappings = () => ({
    relation: {
      modelClass: RelationModel,
    }
  })

would be trivial. I'm willing to add this, but I won't document this as a solution to the circular deps problem, because it only works with babel. I'll simply document this as an optional way to define relationMappings.

[kouak]

I must admit my knowledge regarding how circular dependencies will be handled by v8 once modules land in the codebase is quite limited.

I thank you for your comments, they provided some insight about how babel handle things. It seems this is definitely related to babel though.

I've been able to have a proper behaviour using getters with babel-preset-es2015 :

// RelationModel.js
import MyModel from './MyModel';

export default class RelationModel {
  static get relationMappings() {
    return {
          relation: {
            modelClass: MyModel
          }
        };
  };
};

// MyModel.js
import RelationModel from './RelationModel';

export default class MyModel {
  static get relationMappings() {
    return {
          relation: {
            modelClass: RelationModel
          }
        };
  };
};

// test.js
import RelationModel from './RelationModel';
import MyModel from './MyModel';

// Outputs `RelationModel`
console.log(MyModel.relationMappings.relation.modelClass.name);
// Outputs `MyModel`
console.log(RelationModel.relationMappings.relation.modelClass.name);

However, when using babel-plugin-class-properties :

// MyModel.js
import RelationModel from './RelationModel';

export default class MyModel {
  static relationMappings = {
      relation: {
        modelClass: RelationModel
      }
  };
};

// RelationModel.js
import MyModel from './MyModel';

export default class RelationModel {
  static relationMappings = {
      relation: {
        modelClass: MyModel
      }
  };
};

The same test fails : Cannot read property 'name' of undefined

Now, using your proposal :

// RelationModel.js
import MyModel from './MyModel';

export default class RelationModel {
  static relationMappings = () => ({
      relation: {
        modelClass: MyModel
      }
  });
};

// MyModel.js
import RelationModel from './RelationModel';

export default class MyModel {
  static relationMappings = () => ({
      relation: {
        modelClass: RelationModel
      }
  });
};

// test.js
import RelationModel from './RelationModel';
import MyModel from './MyModel';

// Outputs `RelationModel`
console.log(MyModel.relationMappings().relation.modelClass.name);
// Outputs `MyModel`
console.log(RelationModel.relationMappings().relation.modelClass.name);

Clearly, this issue had more to do with babel and my lack of knowledge about its behaviour than a real issue with Objection.js.

[koskimas]

I think this was a good addition. The reality is that many people use babel and probably never remove it in favor of native ES7 support, so my points were pretty theoretical.