Skip to content

[WIP] Refactor #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
acburdine wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WIP] Refactor #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions app/addon.js
View file
Open in desktop

This file was deleted.

11 changes: 8 additions & 3 deletions config/initializers/node-orm2-connect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,16 @@ export default {
before: 'define-orm-models',
async initialize(application) {
let container = application.container;
let adapter = container.lookup('orm-adapter:node-orm2');
let config = application.config.database;
let config = application.config;

if (!config.database || !config.database.orm2) {
// Config is not specified
return;
Copy link
Contributor

@davewasmer davewasmer Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we probably throw here? If you've included this addon but haven't configured database settings, something must be wrong, right?

}

try {
adapter.db = await fromNode((cb) => orm.connect(config.url, cb));
let connection = await fromNode((cb) => orm.connect(config.database.orm2, cb));
Copy link
Contributor

@davewasmer davewasmer Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Not sure I like scoping the config inside database to orm2. It might be helpful if you have multiple adapters, but most apps will likely have just one.

What if we did something like config.database.orm2 || config.database? Multi-adapter apps could keep everything under database with no special rules for your "main" adapter and "other" adapters, and single-adapter apps could keep their config simple

container.register('database:orm2', connection, { singleton: true });
Copy link
Contributor

@davewasmer davewasmer Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

  1. I don't love storing a connection object in the container. I get why it's necessary, and that's fine. But I'd love to brainstorm a better approach here. The db connection is something I could see sharing across containers (potentially). It's typically managed by the ORM library, and since ava runs tests in parallel, I've noticed that it's very easy to start to hit connection limits (i.e. Error: too many clients) because each individual test creates a db connection. As long as the ORM doesn't store any state on the connection that could be shared across containers as well.

  2. If we do keep the connection object in the container, I'd like to settle on a naming convention for where we put it - I'm okay with database as the type, or connection or something like that.

Copy link
Member

@knownasilya knownasilya Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that we need some conventions around this.

} catch (error) {
application.logger.error('Error initializing the node-orm2 adapter or database connection:');
application.logger.error(error.stack);
Expand Down
6 changes: 3 additions & 3 deletions config/initializers/sync-database.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ export default {
after: 'define-orm-models',
async initialize(application) {
let container = application.container;
let adapter = container.lookup('orm-adapter:node-orm2');
let db = container.lookup('database:orm2', { loose: true });

if (application.config.database.syncSchema) {
await fromNode((cb) => adapter.db.sync(cb));
if (db && application.config.database.sync) {
Copy link
Contributor

@davewasmer davewasmer Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a null check for db needed? Is this in case there is no config defined (from above)? If so, I'd say we throw there and drop the null check here.

await fromNode((cb) => db.sync(cb));
}
}
};
41 changes: 23 additions & 18 deletions app/orm-adapters/node-orm2.js → lib/orm2-adapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ export default class NodeORM2Adapter extends ORMAdapter {
return fromNode((cb) => OrmModel.get(id, cb));
}

queryOne(type, query) {
let OrmModel = this.ormModels[type];
return fromNode((cb) => OrmModel.one(query, cb));
}

all(type, options = {}) {
let OrmModel = this.ormModels[type];
return fromNode((cb) => OrmModel.all(null, options, cb));
Expand All @@ -21,25 +26,25 @@ export default class NodeORM2Adapter extends ORMAdapter {
return fromNode((cb) => OrmModel.find(query, options, cb));
}

findOne(type, query) {
let OrmModel = this.ormModels[type];
return fromNode((cb) => OrmModel.one(query, cb));
}

createRecord(type, data) {
let OrmModel = this.ormModels[type];
return fromNode((cb) => OrmModel.create(data, cb));
}

idFor(model) {
return model.record.id;
}

setId(model, id) {
model.record.id = id;
return true;
}

buildRecord(type, data) {
let OrmModel = this.ormModels[type];
return new OrmModel(data);
}

idFor(model) {
return model.record.id;
}

getAttribute(model, property) {
return model.record[property];
}
Expand Down Expand Up @@ -87,35 +92,35 @@ export default class NodeORM2Adapter extends ORMAdapter {
}

saveRecord(model) {
return fromNode(model.record.save.bind(model.record));
return fromNode((cb) => model.record.save(cb));
}

deleteRecord(model) {
return fromNode(model.record.remove.bind(model.record));
return fromNode((cb) => model.record.remove(cb));
}

defineModels(models) {
// Don't define abstract base classes
models = models.filter((Model) => !(Model.hasOwnProperty('abstract') && Model.abstract));
Copy link
Contributor

@davewasmer davewasmer Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need this here?

Copy link
Member Author

@acburdine acburdine Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done instead in the define-orm-models initializer in Denali itself: https://github.com/denali-js/denali/blob/master/config/initializers/define-orm-models.ts#L21


let db = this.container.lookup('database:orm2');
this.ormModels = {};

// Define models
models.forEach((Model) => {
let attributes = {};
Model.eachAttribute((key, attribute) => {
let modelType = this.container.metaFor(Model).containerName;
Copy link
Contributor

@davewasmer davewasmer Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest denali version introduces Model.getType(container) static method, which is a convenience shortcut for this.

Model.mapAttributeDescriptors((attribute, key) => {
attributes[key] = assign({
mapsTo: this.keyToColumn(key),
type: this.denaliTypeToORMType(attribute.type)
}, attribute.options);
});
this.ormModels[Model.type] = this.db.define(Model.type, attributes);
this.ormModels[modelType] = db.define(modelType, attributes);
});

// Define relationships between models
models.forEach((Model) => {
Model.eachRelationship((key, relationship) => {
let OrmModel = this.ormModels[Model.type];
let modelType = this.container.metaFor(Model).containerName;
Model.mapRelationshipDescriptors((relationship, key) => {
let OrmModel = this.ormModels[modelType];
let Related = this.ormModels[relationship.type];
if (relationship.mode === 'hasOne') {
OrmModel.hasOne(key, Related);
Expand Down
27 changes: 14 additions & 13 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"name": "denali-node-orm2",
"version": "0.0.5",
"scripts": {
"build": "denali build",
"coverage": "scripts/coverage.sh",
"preversion": "scripts/preversion.sh",
"release": "scripts/release.sh",
Expand All @@ -12,31 +13,31 @@
"denali-addon",
"addon"
],
"main": "app/addon.js",
"main": "lib/orm2-adapter.js",
"license": "MIT",
"dependencies": {
"bluebird": "^3.5.0",
"denali": "0.0.x",
"denali": "^0.0.29",
"lodash": "^4.17.4",
"mongodb": "^2.2.25",
"mongodb": "^2.2.30",
"mysql": "^2.13.0",
"orm": "^3.2.3",
"pg": "^6.1.5",
"pg": "^6.4.0",
"sqlite3": "^3.1.4"
},
"devDependencies": {
"ava": "^0.18.1",
"babel-eslint": "^7.2.0",
"babel-plugin-istanbul": "^3.1.2",
"babel-plugin-transform-class-properties": "^6.22.0",
"ava": "^0.20.0",
"babel-eslint": "^7.2.3",
"babel-plugin-istanbul": "^4.1.4",
"babel-plugin-transform-class-properties": "^6.24.1",
"babel-plugin-transform-runtime": "^6.22.0",
"babel-preset-latest": "^6.24.0",
"codeclimate-test-reporter": "^0.4.0",
"babel-preset-latest": "^6.24.1",
"codeclimate-test-reporter": "^0.5.0",
"denali-babel": "0.0.x",
"denali-cli": "0.0.x",
"denali-cli": "^0.0.12",
"denali-eslint": "0.0.x",
"nyc": "^10.1.2",
"standard-version": "^4.0.0"
"nyc": "^11.0.3",
"standard-version": "^4.2.0"
},
"ava": {
"plugins": [],
Expand Down
Loading