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

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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