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

Support explicitly listing migrations, instead of using a filesystem loading function #651

Open
moltar opened this issue Jun 16, 2020 · 8 comments
Labels
s: pending triage Pending Triage
Milestone

Comments

@moltar
Copy link

moltar commented Jun 16, 2020

I am bundling a bunch of code using webpack esbuild to deploy as a lambda function. One of the functions does the migrations. However, because all of the code is bundled as a single file, the result is that node-pg-migrate cannot find the migration files.

E.g. TypeORM supports explicit array of migration classes.

Thanks.

@moltar
Copy link
Author

moltar commented Jul 3, 2020

Would you accept a PR for this?

@dolezel
Copy link
Contributor

dolezel commented Jul 7, 2020

Yes, I will review PR if you create one

@moltar
Copy link
Author

moltar commented Jul 8, 2020

@dolezel this doesn't seem to be too difficult to achieve, but I have one blocker that I want to get you to check on / approve.

Migration class has a migrationPath parameter.

I can see it is used for generating the name of the migration:

this.name = path.basename(migrationPath, path.extname(migrationPath))

This can be worked around by checking the class name, or explicitly providing the migration name (like in TypeORM).

The other use case I found was is in the return value of the main entrypoint (runner) function:

    return toRun.map((m) => ({
      path: m.path,
      name: m.name,
      timestamp: m.timestamp,
    }))

And for this one I don't know if this is being used anywhere, as this can be used by external tools or consuming projects.

Do you think it would be safe to remove the migrationPath parameter requirement for Migration class? We can generate the name externally, and pass the name, rather than the path itself.

Thanks.

@dolezel
Copy link
Contributor

dolezel commented Jul 8, 2020

I think that full name generation can be extracted to function and class can be provided this full name.
I'm not sure if the output of runner is used, but I would assume it is. But it should not be difficult to split full name into path and basename.

@moltar
Copy link
Author

moltar commented Jul 9, 2020

Well, we'd have to drop the path completely as it wouldn't be available when using classes directly.

We can use the path for generating names for the file-based migrations.

But when a class is provided directly, there would be no path.

Just to be clear, this is what I am talking about:

// Migration class
class SomeMigration0123456789 implements MigrationBuilderActions {
  // add a name prop, but it can default to class name (SomeMigration0123456789)
  name = 'my migration'

  up(pgm: MigrationBuilder) {
    // ...
  }

  down(pgm: MigrationBuilder) {
    // ...
  }
}

// runner
migrate({
  databaseUrl: '...',
  direction: 'up',
  // pass an array of migration classes to the runner
  migrations: [
    SomeMigration0123456789
  ]
})

@dolezel
Copy link
Contributor

dolezel commented Jul 10, 2020

Yeah, I think there should be an interface/class for Migration just for up and down methods (and maybe few other things) and also there will be e.g. FileMigration which can extend that class (or maybe have a builder which will read and create it from file definition)

@philjones88
Copy link

This would be really handy when used with something like pkg or ncc where accessing files is a pain.

@littlewhywhat littlewhywhat added the old-label enhancement Functionality that enhances existing features label May 23, 2021
@Shinigami92 Shinigami92 added s: pending triage Pending Triage and removed old-label enhancement Functionality that enhances existing features labels May 2, 2024
@Shinigami92
Copy link
Collaborator

@moltar "array of migration classes" / "FileMigration" sounds like a really interesting feature 👀
However, this is out of scope for v7 (current version that is maintained) and should be reconsidered in v8 or later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: pending triage Pending Triage
Projects
None yet
Development

No branches or pull requests

5 participants