feat: migrations can be provided as a map of loaded modules

[polad]

Currently it's not possible to use node-migrate with packagers relying on static analysis (e.g. webpack) because they need to know all module and file dependencies during bundling. Unfortunately the node-migrate package is dynamically loading migration files in lib/load-migrations.js using require(filepath) where filepath is a variable hence webpack cannot infer during compile time what folders and files to include for bundling in the final output.

This small PR solves the problem by introducing a new migrationsMap property in the options that are provided to the migrate.load(opts, cb) function like so:

const migrationsMap = {
  "1673460432335-create-users-table.js": require("../migrations/1673460432335-create-users-table"),
  "1675438233157-create-sales-table.js": require("../migrations/1675438233157-create-sales-table"),
}

migrate.load({ migrationsMap }, (err, set) => {
  // you callback code here
})

This way there is no need to scan the migrations directory to include the migration files dynamically since they are statically included in the migrationsMap so they will be included by the bundlers like webpack.

The migrationsDirectory is ignored for loading files when the migrationsMap option is provded.

[wesleytodd]

Ah, interesting idea. So I want to say first, while I have in the past used a bundler on server code I don't think it is a good practice. So if we add this I don't think we should do so just because of the bundler use case. That said, I think a strategy to define migrations without dynamic filesystem require/imports is good. Technically you can do it already by using the library exports and not the cli, but I don't think that is very ergonomic.

I think the main thing that is missing here now is sorting. Because map keys are not sorted, to get the "same behavior" for ordering you need to sort the keys before adding them. And two nit picks:

1. Now that we would have two ways to load the migrations I would like to see them broken out into separate functions.
2. Can we just name the option migrations?

[polad]

Thanks for the prompt response. I will try to address the points you raised:

1. while I have in the past used a bundler on server code I don't think it is a good practice.

   Our code runs on serverless AWS Lambdas, so the size of the deployable server-side code matters to us. Without the bundler, all the code with dependencies weighs around 200MB. With the bundler, it's only 1.3MB. AWS Lambda size limit even when you use Layers is 250MB. In any case, I believe loading a smaller code package for spinning up Lambdas is always better. Let me know what you think.

2. a strategy to define migrations without dynamic filesystem require/imports is good. Technically you can do it already by using the library exports and not the cli...

   We only use the migrate API (library exports) to programmatically apply our migrations so the changes proposed in this PR do not affect the CLI. Before these PR changes I tried to use the MigrationSet from the current API along with our custom State Store without using the public migrate.load(). Unfortunately, it's not possible to achieve out of the box since some of the logic that sorts, filters and checks the migrations from the state store and decides which migration should be applied/ignored based on the timestamps etc is buried inside the private loadMigrationsIntoSet() function from the lib/load-migrations.js file which is used by migrate.load() API. And since loadMigrationsIntoSet() dynamically requires() migration files we can't use it as is. I quickly realized I would have to duplicate a lot of code for sorting, filtering, applying/ignoring migrations from loadMigrationsIntoSet(). Am I missing something here?

3. I think the main thing that is missing here now is sorting. Because map keys are not sorted, to get the "same behavior" for ordering you need to sort the keys before adding them...

   The changes proposed in this PR initially did not affect the sorting or filtering logic so it all worked as before and the sortFunction/filterFunction options still apply. The way migrations are ordered in the migrationsMap is irrelevant for sorting. But since I broken out the migration loading into separate function per your suggestion I had to do some refactoring around sorting and filtering although the behaviour is still intact and should work as before.

4. Now that we would have two ways to load the migrations I would like to see them broken out into separate functions.

   I added this in the latest commit. Is this what you meant?

5. Can we just name the option migrations?

   We sure can but I thought migrations alone would sound too abstract and migrationsMap is more explicit since it's a map. Let me know if you're not comfortable with migrationsMap I can rename it to migrations.

[wesleytodd]

Let me know what you think.

That makes sense. I have run this tool as my app starts up before, just not in lambda. Are you running them when each lambda instances spins up? Not bad, just interesting for me to learn and understand how folks are using the lib.

Unfortunately, it's not possible to achieve out of the box

Ah, yeah that makes sense. Honestly I would say that is a failure of the design of that part. A long time ago I started a refactor which should have fixed these issues, just never had the time to finish that. Would love to see that as a PR sometime if you were ever interested.

The changes proposed in this PR initially did not affect the sorting or filtering logic so it all worked as before and the sortFunction/filterFunction options still apply.

Ah, maybe I missed something there. I can take another look. I guess as long as there are tests for both which meet the same result it should be good.

Is this what you meant?

Yep!

We sure can but I thought migrations alone would sound too abstract and migrationsMap is more explicit since it's a map. Let me know if you're not comfortable with migrationsMap I can rename it to migrations.

It is not a Map, so yeah I would prefer it be called migrations. Honestly I almost did say it should be a sorted array, but was on the fence and am not sure it matters that much because if it was an array we would need to use something else to name them (like a name key) which would add complexity over just passing an object.

[polad]

Are you running them when each lambda instances spins up?

Are you asking when do we run webpack? We use Continuous Deployment pipeline where each new release gets bundled using webpack and packaged by serverless framework into a zip archive before being deployed to production. We also use webpack's "source-maps" options to get proper error stack traces.

If you're asking when we run migrate lib then we have different strategies depending on the microservice. For some we run the db migrations on lambda startup for others we have a dedicated migrate lambda function that we invoke during the deployment of the new release.

It is not a Map, so yeah I would prefer it be called migrations.

Technically javascript Objects are Maps where properties are string keys. And we've all been using Objects as maps in javascript before ES6 introduced the actual Map type :) In any case, I renamed the option to migrations it's a shorter name so all good :) I also updated the Readme.md with this newly introduced option.

How do we proceed further? Can we merge the PR?

[wesleytodd]

If you're asking when we run migrate lib

Yeah that is what I was asking. Because bundling this code only makes sense if you need to run it in the lambda. Anyway, not a big deal and I agree this is a good feature.

Objects are Maps

Yes, but now that there is an actual implementation called map, using that as a suffix typically implies it should be a Map instance. Since this is not that I don't see a reason to include the suffix.

Can we merge the PR?

Taking a look, but yeah I think it should be good.

[wesleytodd]

Suggested change

	- `migrations`: An object where keys are migration file names and values are migration modules loaded with `require()`
	- `migrations`: An object where keys are migration names and values are migration objects

I think they can be loaded however you want, you can define them inline if you want. I don't think we need to specify "with require"

[polad]

Fair point.

[wesleytodd]

Made a couple of final comments. But yeah, other than those I can merge and release this.

[polad]

@wesleytodd I applied you suggestions in my latest commits. Let me know if there is anything else.

Thanks again

[wesleytodd]

Hey, sorry busy day. I probably wont be able to get to this tonight but will try and look in the morning.

[polad]

Hey @wesleytodd :) whenever you get a chance to take another look. Thanks 👍

[polad]

hey @wesleytodd just bumping this PR as a friendly reminder :) there are few minor changes really. Thanks

[wesleytodd]

Sorry looks like I dropped the ball on this. Taking a look now.

[wesleytodd]

Looks like I still can't change the settings to allow the actions to run on this PR. @tj it would be great if you could make me an administrator on this repo. I almost moved it into my account once before because of this, maybe I should just do that. I will pull it down and manually run the tests and stuff I guess.

[wesleytodd]

Released as 2.1.0