[WIP] Refactor #16
[WIP] Refactor #16
Conversation
|
@davewasmer this should be good to go now :) |
no issue BREAKING CHANGE: database config is now provided via the `database.orm2` property rather than just the database.url property - make orm2-adapter the main export of the addon - cleanup adapter for latest denali changes - update dependencies - add build script
There was a problem hiding this comment.
Looks awesome, thanks for keeping this up to date @acburdine! Just a couple items to tweak potentially.
|
|
||
| if (!config.database || !config.database.orm2) { | ||
| // Config is not specified | ||
| return; |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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
| try { | ||
| adapter.db = await fromNode((cb) => orm.connect(config.url, cb)); | ||
| let connection = await fromNode((cb) => orm.connect(config.database.orm2, cb)); | ||
| container.register('database:orm2', connection, { singleton: true }); |
There was a problem hiding this comment.
Two things:
-
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. -
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
databaseas the type, orconnectionor something like that.
There was a problem hiding this comment.
Agreed that we need some conventions around this.
|
|
||
| if (application.config.database.syncSchema) { | ||
| await fromNode((cb) => adapter.db.sync(cb)); | ||
| if (db && application.config.database.sync) { |
There was a problem hiding this comment.
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.
|
|
||
| defineModels(models) { | ||
| // Don't define abstract base classes | ||
| models = models.filter((Model) => !(Model.hasOwnProperty('abstract') && Model.abstract)); |
There was a problem hiding this comment.
Don't we need this here?
There was a problem hiding this comment.
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
| models.forEach((Model) => { | ||
| let attributes = {}; | ||
| Model.eachAttribute((key, attribute) => { | ||
| let modelType = this.container.metaFor(Model).containerName; |
There was a problem hiding this comment.
Latest denali version introduces Model.getType(container) static method, which is a convenience shortcut for this.
Pushing this branch up - just need to clean it up and it should be good to go 👍