Skip to content

[question]: why does the emission happen after the migration? #5

Description

@Rignchen

In the code we can see

      // Perform the migration
      await this.runTransaction(async () => {
        for (const migration of pendingMigrations) {
          try {
            // Apply migration
            await migration.up(this.db, ...args);

            // Record migration
            await this.recordMigration(migration.name, nextBatch);
            appliedMigrations.push(migration.name);
          } catch (err) {
            throw new MigrationExecutionError(`Failed to apply migration "${migration.name}"`, err as Error);
          }
        }
      });

      for (const migrationName of appliedMigrations) {
        this.emit('migration:applied', migrationName, nextBatch);
      }

and

      // Perform the rollback
      await this.runTransaction(async () => {
        for (const row of rows) {
          const migration = this.migrations.find((m) => m.name === row.name);
          if (!migration) {
            throw new MigrationFileError(`Migration "${row.name}" not found.`);
          }

          try {
            // Revert migration
            await migration.down(this.db, ...args);

            // Remove migration record
            await this.removeMigration(migration.name, currentBatch);

            appliedMigrations.push(migration.name);
          } catch (err) {
            throw new MigrationExecutionError(`Failed to rollback migration "${migration.name}"`, err as Error);
          }
        }
      });

      for (const migrationName of appliedMigrations) {
        this.emit('migration:rollback', migrationName, currentBatch);
      }

I personally find that confusing, why do you want for all transactions to be done before emitting that a migration is done? why not

      // Perform the migration
      await this.runTransaction(async () => {
        for (const migration of pendingMigrations) {
          try {
            // Apply migration
            await migration.up(this.db, ...args);

            // Record migration
            await this.recordMigration(migration.name, nextBatch);
            appliedMigrations.push(migration.name);

            // Notify user
            this.emit('migration:applied', migration.name, nextBatch);
          } catch (err) {
            throw new MigrationExecutionError(`Failed to apply migration "${migration.name}"`, err as Error);
          }
        }
      });

and

      // Perform the rollback
      await this.runTransaction(async () => {
        for (const row of rows) {
          const migration = this.migrations.find((m) => m.name === row.name);
          if (!migration) {
            throw new MigrationFileError(`Migration "${row.name}" not found.`);
          }

          try {
            // Revert migration
            await migration.down(this.db, ...args);

            // Remove migration record
            await this.removeMigration(migration.name, currentBatch);

            appliedMigrations.push(migration.name);

            // Notify user
            this.emit('migration:rollback', migration.name, nextBatch);
          } catch (err) {
            throw new MigrationExecutionError(`Failed to rollback migration "${migration.name}"`, err as Error);
          }
        }
      });

I think this would make more sens and I am curious of the reason why you didn't choose to so it like that

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions