node-sqlite icon indicating copy to clipboard operation
node-sqlite copied to clipboard

Add Migrations class

Open Roma-Kyrnis opened this issue 1 year ago • 7 comments

For this, I needed to

  • bump the version of the prettier-standard
  • add optional property filename to IMigrate.MigrationData

Other changes happen because I am supposed to the prettier-standard version bump or because of pre-commit commands

Roma-Kyrnis avatar Jan 21 '24 17:01 Roma-Kyrnis

Thanks for this, this is a really great start.

theogravity avatar Jan 22 '24 03:01 theogravity

You should be able to delete the migrate.ts file and in the Database class, you can do:

  async migrate (config?: MigrationParams) {
    const migration = new Migrations(...)
    // call migrate to latest
  }

Edit: Since we're doing a major version bump, we can introduce breaking changes.

Let's remove that method entirely, and we'll direct users to use the new class instead

theogravity avatar Jan 22 '24 03:01 theogravity

Ok, I've made all the comments for the first round. Looking forward to the changes!

theogravity avatar Jan 22 '24 03:01 theogravity

So If we are making breaking changes. I have an idea to discuss with you: Our migrate - initMigration function will look like:

async migrate (): Promise<void> {
    const migrations = this.config.migrations ?? (await this.readMigrations())
    this.config.migrations = migrations

    // Create a database table for migrations meta data if it doesn't exist
    await this.createMigrationsTable()
  }
  1. It's the first variant to have "stable" migrations variable in the config to not call await this.readMigrations() on every down or up
  2. We can create static method to run const migrations = this.config.migrations ?? (await this.readMigrations()) asyncronously
  3. Get const migrations = this.config.migrations ?? (await this.readMigrations()) migrations variable on every up or down operation
  4. User can retrieve migrations using the readMigrations method and set migrations variable on class creation

Roma-Kyrnis avatar Jan 24 '24 14:01 Roma-Kyrnis

We shouldn't make this.config.migrations part of the Database constructor config

createMigrationsTable shouldn't be part of Database - it's specifically a migration method and should be in the migration class instead

if you need to do some kind of initialization, then add a protected async init() method to your migration class

If there are async calls that must be done when the migration class is created, then do the following:

  • Make the constructor of the migration class private
  • Create a static method on the migration class called static async initMigration(config) that does:
    • Creates a new instance of the migration class, passing the config to it
    • Then calls the async init() method to initialize
    • Returns the new instance of the migration class

You can see the pattern here:

https://stackoverflow.com/questions/51134172/what-is-the-usage-of-private-and-protected-constructors-in-typescript

theogravity avatar Jan 27 '24 23:01 theogravity

Hi Theo, I'll go tomorrow to the base training. I'll not have a laptop with me for 1-2 months. I'll push in 1-2 hours last updates before I leave.

May I continue to develop Migrations later?

It'll be great if someone wants to bring his impact here! If not I'll finish development later

Roma-Kyrnis avatar Jan 28 '24 14:01 Roma-Kyrnis

Hi Theo, I'll go tomorrow to the base training. I'll not have a laptop with me for 1-2 months. I'll push in 1-2 hours last updates before I leave.

May I continue to develop Migrations later?

It'll be great if someone wants to bring his impact here! If not I'll finish development later

Take as much time as needed!

theogravity avatar Jan 28 '24 18:01 theogravity