neumino / thinky

JavaScript ORM for RethinkDB
http://justonepixel.com/thinky/
Other
1.12k stars 128 forks source link

Circular Dependency Issue with ES2015 / Babel 6 #399

Open dacodekid opened 8 years ago

dacodekid commented 8 years ago

Following thinky's recommended architecture,

\\ project.js
import { thinky, type } from 'app/util/thinky';

const Project = thinky.createModel('Project', {
  id: type.string(),
  name: type.string(),
  statusId: type.string()
});

export { Project };

import Status from 'app/models/lookup/status';
Project.belongsTo(Status, 'status', 'statusId', 'id');
\\ status.js
import { thinky, type } from 'app/util/thinky';

const Status = thinky.createModel('Status', {
  id: type.string(),
  name: type.string()
});

export { Status };

import Project from 'app/models/core/project';
Status.hasMany(Project, 'projects', 'id', 'statusId');

I'm getting throw new Error("First argument ofhasManymust be a Model") error. Converting this code with require works fine. It only produces the error if I add the relationship. So I couldn't come to a conclusion that it is babel's issue either. Would appreciate your input.

neumino commented 8 years ago

It's probably the same issue as https://github.com/neumino/thinky/issues/351

dacodekid commented 8 years ago

@neumino, I saw that b4 posting but couldn't relate the error I'm receiving. I even tried to move all the relationships to another js file, but that didn't help either. It works only if I place all my models into a single file.

neumino commented 8 years ago

Can you provide a full example with 2 models that raises this error?

dacodekid commented 8 years ago

@neumino , just pushed it. Plz take a look at this repo. https://github.com/dacodekid/hpa

ondreian commented 8 years ago

shouldn't you be using:

import { Project } from 'app/models/core/project';

since you aren't exporting Project as the default?

dacodekid commented 8 years ago

@ondreian that didn't fix either. I was trying to import every way think of.

ondreian commented 8 years ago

@dacodekid yeah, sorry bud. was just the first thing I saw that looked like it might have been the problem without looking at your app architecture. Hope you figure it out.

neumino commented 8 years ago

I don't use es6 stuff, but after skimming over the docs, it seems that imports are hoisted (and are actually bindings), so this doesn't work. I don't know what the standard way to work around, but I guess you can export a function that will create the relation and call that function at the appropriate place. Or you can just use require.

neumino commented 8 years ago

Reopening I'll write some docs later. I need to properly test thing though

dacodekid commented 8 years ago

I guess you can export a function that will create the relation and call that function at the appropriate place

That's what I exactly did at first. But then, wrapping the relationship inside model's ready promise seems to work. I haven't test the code, but at least I'm not getting the error. If you think this might be a workaround, plz close this issue.

UPDATE: Spoke too soon I guess. The below code doesn't seems to work. Need more test.

import {
  thinky, type
}
from 'app/util/thinky';

const Status = thinky.createModel('Status', {
  id: type.string(),
  name: type.string()
});

Status.ready().then(() => {
  Status.hasMany(thinky.models.Project, 'projects', 'id', 'statusId');
});

export {
  Status
};
import {
  thinky, type
}
from 'app/util/thinky';

const Project = thinky.createModel('Project', {
  id: type.string(),
  name: type.string()
});

Project.ready().then(() => {
  Project.belongsTo(thinky.models.Status, 'status', 'statusId', 'id');
});

export {
  Project
};
dacodekid commented 8 years ago

This is the only workaround I found. Moving on with this until find a better one.

import {
  thinky, type
}
from 'app/util/thinky';

const Project = thinky.createModel('Project', {
  id: type.string(),
  name: type.string()
});

Project.relationship = () => {
  Project.belongsTo(thinky.models.Status, 'status', 'statusId', 'id');
};

export {
  Project
};
erik-singleton commented 8 years ago

I ended up writing

let Status = require('./Status').default;
User.hasMany(Status, 'statuses', 'id', 'statusId');

But I might like your solution more

samkelleher commented 8 years ago

To jump in here, after using ES6 a lot and studying the proposed architecture from the docs, I've concluded that thinky is incompatible with new JavaScript module system and recommend using the require only.

This is due to the fact that the import statements are called immediately, so it's impossible to have two models depend on each other in a circular reference as simply importing them at startup will cause either one to fail because the other hasn't been created yet.

It also means that because the imports are called immediately, the model creation requires an instance of thinky, and because thinky connects to the database server as soon as it's created, this creates an undesirable situation where the application will actually begin establishing a connection during the initial tree setup of your application. So if you need to do any preparation or initialization in your application before establishing a database connection (like load config and connection settings), you can't.

evenfrost commented 8 years ago

So, what's the full scenario here to utilize Thinky with ES6 imports? I've ended up with following:

// /utils/thinky.js
import Thinky from 'thinky';
export default new Thinky();

// /models/foo.js
import thinky from '../utils/thinky';

const { type, r } = thinky;

const {
  string,
  number,
  date,
} = type;

const Foo = thinky.createModel('Foo', {
  id: string(),
  name: string().required(),
  type: string(),
});

export { Foo };

// /anything.js
import { Foo } from './models/foo';
// use Foo model here

Are there any lines that could be optimized?

neumino commented 8 years ago

Use require? What does the ES6 import give you that require doesn't?

evenfrost commented 8 years ago

For me this is possibility to use full ES6+ environment and have cleaner code as well. And following this logic, i.e. why not just use require, we could still write full ES5 and not bothering at all, but we all want to progress aren't we?