experimentalAutoMigrate (draft)

[vmasdani]

No description provided.

[eveningkid]

Hey,

I know there are many comments but I hope they can be helpful to you!

Have a great week :)

[eveningkid]

The rest of the codebase uses Error usually:

throw new Error("Auto-migration only works on SQL.");

[eveningkid]

I think it would be great to combine createTable and createTableOnlyTable together (I'm sure you thought of doing it!).

Here's an idea:

static async createTable(options = { withFields: true }) {
  const createQuery = this._options.queryBuilder
    .queryForSchema(this)
    .table(this.table);
  
  if (options.withFields) {
    // NOTE: the queryBuilder.createTable signature should be changed to match the changes
    createQuery.createTable({
      fields: this.formatFieldToDatabase(this.fields) as ModelFields,
      defaults: this.formatFieldToDatabase(this.defaults) as ModelDefaults,
      withTimestamps: this.timestamps,
      ifNotExists: true,
    });
  } else {
    createQuery.createTable({
      withTimestamps: this.timestamps,
      ifNotExists: true,
    });
  }
  
  await this._options.database.query(createQuery.toDescription());
  
  this._isCreatedInDatabase = true;
}

[eveningkid]

Just a quick note: I think field would be more appropriated here

[eveningkid]

This could be more compact (but is it possible?):

const alterQuery = this._queryBuilder
  .table(this.table)
  .addColumn(
    this.formatFieldToDatabase(key) as string,
    // vvv I wonder if this might not be an object sometimes? with autoIncrement, primary, etc?
    this.fields[key].toString()
  )
  .toDescription();

If I can give a piece of advice: when creating functions/methods/etc. I personally think of what I would ideally type first, then of the implementation.

That way, you keep your API simpler (because that's how you'd ideally have written it in the first place!)

[eveningkid]

I'm just thinking of another approach: we could SELECT * FROM table and see what fields we get in return?

Because with the current implementation, we're making a lot of queries. If we could just make one query, see what fields we have and the ones missing would then be created with one ALTER query too.

// User { name, age, address }
// See how this could be created. This is just an example
const fieldsInDatabase = {
  name: true,
  age: true,
};

const missingFields = [];

for (const field of this.fields) {
  if (field in fieldsInDatabase) {
    continue;
  }
  
  const fieldType = ...;
  
  missingFields.push({ field, type: fieldType });
}

if (missingFields.length === 0) {
  return;
}

const addMissingFieldsQuery = this._queryBuilder
  .table(this.table)
  .addColumns(missingFields)
  .toDescription();

await this._options.database.query(addMissingFieldsQuery);

This I guess would run faster especially if there are many models, what do you think?

[eveningkid]

Based on previous comment, could be combined with createTable

[eveningkid]

I think this one isn't necessary, but I'm sure you had a problem somewhere when generating the query?

[eveningkid]

Could queryBuilder be used there instead?

[eveningkid]

I remember you found a way to solve this with dex :D

[eveningkid]

So this could be:

await model.createTable({ withFields: false });