Skip to content
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.

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

Re: experimentalAutoMigrate #316

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

DomThePorcupine
Copy link

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

Copy link
Owner

@eveningkid eveningkid left a comment

Choose a reason for hiding this comment

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

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
Comment on lines 233 to 235
if (dialect === "mongo") {
throw new Error("Auto-migration only works on SQL.");
}
Copy link
Owner

Choose a reason for hiding this comment

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

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 */
Copy link
Owner

Choose a reason for hiding this comment

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

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
Comment on lines 187 to 189
Object.keys(this.fields).map((f) => {
fieldsToCreate[f] = true;
});
Copy link
Owner

Choose a reason for hiding this comment

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

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
Comment on lines 192 to 195
const colQuery = this._queryBuilder.table(this.table).limit(1).get();
const columns = await this._options.database.query(
colQuery.toDescription(),
) as Model[];
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe replace colQuery with columnsQuery as we're using columns on the next line

lib/model.ts Outdated
Comment on lines 202 to 204
Object.keys(columns[0]).map((existing) => {
delete fieldsToCreate[existing];
});
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above, for in might be more readable:

for (const existingField in columns[0]) {
  delete fieldsToCreate[existingField];
}

Comment on lines 136 to 150
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;

Copy link
Owner

Choose a reason for hiding this comment

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

This is not necessary

@@ -60,6 +61,8 @@ export type QueryDescription = {
fieldsDefaults?: ModelDefaults;
timestamps?: boolean;
values?: Values | Values[];
addColumn?: string;
Copy link
Owner

Choose a reason for hiding this comment

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

Let's rename this to columnName. I think it makes more sense as we use it with columnType

Comment on lines 95 to 105
addColumn(columnName: string) {
this._query.type = "alter";
this._query.addColumn = columnName;
return this;
}

columnType(columnType: string) {
this._query.columnType = columnType;
return this;
}

Copy link
Owner

Choose a reason for hiding this comment

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

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
Comment on lines 230 to 231
.addColumn(this.formatFieldToDatabase(field) as string)
.columnType(fieldType.toString())
Copy link
Owner

Choose a reason for hiding this comment

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

So this will be replaced to:

.addColumn(this.formatFieldToDatabase(field) as string, fieldType.toString())

Comment on lines 49 to 51
console.log(
`[autoMigrate] QUERY DETECTED AS ALTER TABLE: ${query.table} ${query.addColumn} ${query.columnType} ${this._dialect}`,
);
Copy link
Owner

Choose a reason for hiding this comment

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

This is not needed I think

@DomThePorcupine
Copy link
Author

DomThePorcupine commented Dec 30, 2021

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 deno fmt.

There's a few things I think maybe we should discuss:

  • It seems like we rely heavily on dex, but the one in the deno.land third party library links to a github repo that doesn't exist. Perhaps we should consider forking so we can maintain it ourselves?
  • I couldn't get the dropColumn function to work, and was going off of the knex docs, I'm not sure if it's because dex didn't implement properly or if there's a bug somewhere ☹️ either way, I'm sorry but if you've got any ideas I'd greatly appreciate it.
  • I added a test and noticed that I got a deprecated warning, it seems like the tests are using a v1 style database instantiation. Maybe I can update them at a later time, but what do you think about adding test running and fmt running as github actions or something?

Thanks for all your help!
Dom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants