tj / node-migrate

Abstract migration framework for node
MIT License
1.54k stars 224 forks source link

Required fixes for #144 #171

Closed msanguineti closed 3 years ago

msanguineti commented 4 years ago

I have made some fixes as required by @wesleytodd in #144 and also updated the README to reflect the changes. Closes #105 #144

msanguineti commented 4 years ago

Regarding the deprecation/major version issue, I tend to agree with you. I do not know what is your release cycle for this project or if you even have one, but you could set apart this PR and I can open another one with just the deprecation warning, no new flag. Then, at your convenience, you could merge this PR minus the compiler code and all that.

Also, @hudovisk introduced a whole new dep just for the deprecation warning. While this might be fine, a believe that your logging system would suffice. Of course, if you decide to release as minor/patch with the deprecation only, then the dep will go away together with the deprecated code in the major.

That's up to you to decide.

wesleytodd commented 4 years ago

Regarding the deprecation/major version issue, I tend to agree with you. I do not know what is your release cycle for this project or if you even have one, but you could set apart this PR and I can open another one with just the deprecation warning, no new flag.

No need for a new PR. I think I will push this as a major despite there being no other pending breaking changes.

Also, @hudovisk introduced a whole new dep just for the deprecation warning.

This particular dep has some extra stuff for better deprecation messaging, so I would like to keep it.


I will hopefully take a look through the other open PRs soon and prepare a release with this.

msanguineti commented 4 years ago

@wesleytodd I added the require and compiler flags also for programmatic use of the library (in the options to pass to migrate.load. I needed them for some e2e testing using jest, ts-jest and mongodb-memory-server. I also amended the README to reflect those changes in the API.

wesleytodd commented 4 years ago

Interesting, while I am not opposed to this addition, I wonder why you would use it if you are using the JS interface. You could just require the file on your own before requiring migrate, right?

msanguineti commented 4 years ago

I have everything in typescript: tests, source, migrations. What I do is to have a jest setup file which starts mongo and runs migrations on the "new" db. jest (ts-jest) will not transpile any file called from the setup. Now you see the problem... If I write everything in JS, then I have problems since the migrations are importing source files. So, the solution is either to compile everything and then run the tests, or just require ts-node/register which will take care to transpile the things that jest stubbornly won't do.

This is the programmatic use of load (in a typescript file):

...
  migrate.load(
      {
        stateStore: new Store(),
        migrationsDirectory: 'src/migrations',
        // compiler: 'ts:./test/config/ts-compiler.js',
        require: 'ts-node/register',
        filterFunction: (name: string) => /^[0-9]+-.*/.exec(name),
      },
      (
        err: unknown,
        set: {
          up: (arg0: (error: unknown) => Promise<void>) => void
        }
      ) => {
        if (err) {
          reject(err)
        }
        set.up(async (error: unknown) => {
          if (error) {
            reject(error)
          }
          console.log('running up')
          const connection = await DB.connection
          await connection.close()
          resolve()
        })
      }
    )
...

The weird open/close connection is to stop jest hanging... I am seriously thinking to move the whole migration framework (and possibly the testing too) to vanilla JS but I have some issues (as already mentioned) that require some serious thinking.

wesleytodd commented 4 years ago

I still don't think I understand the use case. Could you maybe break this into two PRs so I can merge the original work first?

msanguineti commented 4 years ago

Reverted latest commit so now you only have the original changes.