Re: experimentalAutoMigrate #316
Conversation
…te-automigrate
There was a problem hiding this comment.
Hi Dom,
First of, thank you so much for looking into this :)
I just found some time to review the changes and add extra comments for what I missed in the previous PR.
I know there is a lot, but most changes as you'll see are to keep everything as consistent as possible :)
Another important thing I haven't mentioned: please run deno fmt before committing. It'll apply the coding style used in deno projects.
If you have any questions or opinions on any of the above, do not hesitate to share them!
Have a great holiday!
lib/database.ts
Outdated
| if (dialect === "mongo") { | ||
| throw new Error("Auto-migration only works on SQL."); | ||
| } |
There was a problem hiding this comment.
This could be moved upper, right before the for loop (as we'd already know it's not possible)
lib/model.ts
Outdated
|
|
||
| this._isCreatedInDatabase = true; | ||
| } | ||
|
|
||
| /** Auto-migrate */ |
There was a problem hiding this comment.
Could we have a more explicit comment here?
As a reference, here's the one for Model.createTable:
/** Create a model in the database. Should not be called from a child model. */
lib/model.ts
Outdated
| Object.keys(this.fields).map((f) => { | ||
| fieldsToCreate[f] = true; | ||
| }); |
There was a problem hiding this comment.
I tend to find for in loops more readable, what do you think?
Object.keys(this.fields).map((f) => {
fieldsToCreate[f] = true;
});
// vs
for (const field in this.fields) {
fieldsToCreate[field] = true;
}
lib/model.ts
Outdated
| const colQuery = this._queryBuilder.table(this.table).limit(1).get(); | ||
| const columns = await this._options.database.query( | ||
| colQuery.toDescription(), | ||
| ) as Model[]; |
There was a problem hiding this comment.
Maybe replace colQuery with columnsQuery as we're using columns on the next line
lib/model.ts
Outdated
| Object.keys(columns[0]).map((existing) => { | ||
| delete fieldsToCreate[existing]; | ||
| }); |
There was a problem hiding this comment.
Same as above, for in might be more readable:
for (const existingField in columns[0]) {
delete fieldsToCreate[existingField];
}
lib/translators/sql-translator.ts
Outdated
| case "alter": | ||
| console.log( | ||
| `[autoMigrate] QUERY DETECTED AS ALTER TABLE: ${query.addColumn} ${query.columnType}`, | ||
| ); | ||
| console.log( | ||
| "[autoMigrate]", | ||
| queryBuilder?.toString(), | ||
| // ?.schema | ||
| // queryBuilder?.createTable | ||
| // ?.alterTable(query.table, (table: any) => {}) | ||
| // ?.toString() | ||
| ); | ||
|
|
||
| break; | ||
|
|
lib/query-builder.ts
Outdated
| @@ -60,6 +61,8 @@ export type QueryDescription = { | |||
| fieldsDefaults?: ModelDefaults; | |||
| timestamps?: boolean; | |||
| values?: Values | Values[]; | |||
| addColumn?: string; | |||
There was a problem hiding this comment.
Let's rename this to columnName. I think it makes more sense as we use it with columnType
lib/query-builder.ts
Outdated
| addColumn(columnName: string) { | ||
| this._query.type = "alter"; | ||
| this._query.addColumn = columnName; | ||
| return this; | ||
| } | ||
|
|
||
| columnType(columnType: string) { | ||
| this._query.columnType = columnType; | ||
| return this; | ||
| } | ||
|
|
There was a problem hiding this comment.
This could be combined into one method addColumn because whenever we add a column, we'll set the type:
addColumn(columnName: string, columnType: string) {
this._query.type = "alter";
this._query.columnName = columnName;
this._query.columnType = columnType;
return this;
}
lib/model.ts
Outdated
| .addColumn(this.formatFieldToDatabase(field) as string) | ||
| .columnType(fieldType.toString()) |
There was a problem hiding this comment.
So this will be replaced to:
.addColumn(this.formatFieldToDatabase(field) as string, fieldType.toString())
lib/translators/sql-translator.ts
Outdated
| console.log( | ||
| `[autoMigrate] QUERY DETECTED AS ALTER TABLE: ${query.table} ${query.addColumn} ${query.columnType} ${this._dialect}`, | ||
| ); |
|
Hey :) Thanks so much for taking the time to give me feedback! I've gone through and made the requested changes, as well as adding some new stuff and running There's a few things I think maybe we should discuss:
Thanks for all your help! |
Hi all,
I'm trying to pick up where PR #259 left off a few months ago. I made some of the requested changes, and was hoping to go over a few of the others in some greater detail.
All the heavy lifting was done by @vmasdani I am just trying to help out to get this change merged :)
Thanks,
Dom