FileStore.save TS typing is misleading

[Woodz]

Currently the typing of FileStore.save is

  save(set: MigrationSet, cb: CallbackError): void;

where CallbackError is typed as

type CallbackError = (err: any) => void;

This is very confusing as cb: CallbackError implies that this a callback to be invoked only if an error occurs. Whereas migrate requires this callback to be invoked with null if no error has occurred to proceed with future migrations.

Based on the actual implementation of Filestore.save:

FileStore.prototype.save = function (set, fn) {
  fs.writeFile(this.path, JSON.stringify({
    lastRun: set.lastRun,
    migrations: set.migrations
  }, null, '  '), fn)
}

we can see that this callback should match the signature returned by fs.writeFile, which is:

callback: (err: NodeJS.ErrnoException | null, written: number, str: string) => void

So CallbackError should be typed as

type CallbackError = (response: Error | null) => void;

and therefore I propose renaming in the TS typings to Callback or similar and documented accordingly

[LinusU]

Yeah, the current typings doesn't look great. But I don't think that we should change err to response. Going with Node.js callback convention the first parameter is the error and the second one the result. Since this is a void function the result is always going to be undefined. But I don't think that that means that we should rename the error to response.

How about type Callback = (err: Error | null) => void?

[wesleytodd]

I made a comment in https://github.com/tj/node-migrate/pull/196. I do agree this is more technically correct, but I really am not a fan of pushing a breaking change with just this change. Let's consolidate the conversation in the PR so we can decide what to do.