[Feature Request / Bug] Don't process.exit(1) in the API; Throw an error instead

[Flaque]

Disclaimer: I may be totally off base on what's actually happening here, if I am please let me know 😅

Overview

The API shouldn't quit the process, it should throw an error so that clients can catch the issue.

Issue

When the API fails, it sometimes throws a process.exit(1) which can lead testing frameworks (Jest) to misplace blame. Since Jest (and Ava) run tests in parallel, process.exit(1) can stop the whole shebang in the middle of something leading to weird errors like this:

For Screenreaders:

PASS src/lib/frame/test/frame.test.js
RUNS src/lib/core/test/core.test.js
error Command failed with exit code 1.

In the example above, a test that hadn't reported in yet may have called the process.exit(1), but since that interrupted the output of src/lib/core/test/core.test.js, it appears as if that test is the cause.

At the moment, it looks like the api.js node-db-migrate is calling the commands directly via load.

Take the create command, as reproduced below:

create: function (migrationName, scope, callback) {
    var executeCreateMigration = load('create-migration');  // This right here being loaded in
    // ... Other stuff
},

That will call the lib/commands/create-migration.js file, which calls process.exit(1) in several places. For example, if it fails to create a migration:

  createMigrationDir(migrationsDir, function (err) {
    var index = require('../../connect');
    var Migration = require('../migration.js');

    if (err) {
      log.error('Failed to create migration directory at ', migrationsDir, err);
      process.exit(1);
    }

    // ...
  }

Solution

Whenever we would call process.exit(1), check if we're running in the API and instead throw an error? Or maybe redefine process.exit to something like this:

const oldProcessExit = process.exit;

// Before API defs
process.exit = (code) => {
  throw new Error(`Something went wrong in db-migrate! Error code: ${code}`); 
}

// After API defs
process.exit = oldProcessExit;

---

If this is okay, I would be willing to take a stab at it. If not, no worries.

[wzrdtales]

No need to fix. Just tell db-migrate to not register the events on uncatched{Rejection,Exception}.

The options property you need to set for this is, throwUncatched, see also here:

node-db-migrate/api.js

Line 42 in 4de822b

if (options && !options.throwUncatched) load('helper/register-events')();

[wzrdtales]

Ah and I just saw the createMIgrationsDir example, that should be probably just removed though.

[Flaque]

Thanks so much! ❤️

I added a PR to the docs to add this option. (If that was the wrong place, let me know). Thanks for the help!

[wzrdtales]

My pleasure, and thank you for contributing on the docs <3

I will also push out a change to replace the aforementioned exit.

[Flaque]

<3

[artemave]

Hats off to @Flaque's diplomacy skills. I, on the other hand, am pretty speechless.

Because I just spent a number of minutes figuring as to why the following code

(async () => {
  const dbmigrate = require('db-migrate')

  process.env.DATABASE_URL = 'postgres://localhost:5432'

  const dbm = dbmigrate.getInstance(true)
  await dbm.dropDatabase('pr-apps-test')
  await dbm.createDatabase('pr-apps-test')
  await dbm.up()
})().catch(e => {
  console.error(e)
})

Ends up with

And what do I find in the end? process.exit(1) inside a library code.

[Flaque]

:'( Sorry you ran into that @artemave. Glad you found the issue though!

[wzrdtales]

@artemave db-migrate is not a library, but lately also a library ;) So for programmatic modes, it could happen that some stuff should not be where it is. I am not sure though, what you wanted to reach with your comment here. Which is not a bug report, nor a feature suggestion, nor you seem to want to help out or something. It feels more like you just wanted to brag around here, I am sorry if you did not wanted to, but this clearly reads like it. I want to friendliest point yout to the COC.

So you did ran into the issue of public registers right? Can't say since you did not provided a proper problem description, though. Just correct me should I am be wrong. However, when @Flaque opened this issue, I'm not quite sure if I respected the programmatic mode at that time, I agree however, that for that mode we should default to not register the error handlers.

[artemave]

A library must not:

• call process.exit
• call console.log

This library does both and that is about as most accurate description of the problem as I can come up with.

[wzrdtales]

@artemave DB-Migrate was once just a cli tool and now started to provide abilities to be used as a module. You're doing nothing more than wasting time, if you want to actually help the open source community, be constructive and helpful. Yes bugs exists, problems exist and all is fine. But I see I was right, you do want only brag around here, and with that I kindly again refer you to the COC.

I am more than happy to accept bug reports from you, or even better, see you actually helping the open source world and contributing PRs, which would be awesome!

After all this is open source, and we, the whole community and I really mean no single other open source project of the world is due anyone anthing.

Since I do not waste time on trolling or toxicity too much, this will not receive any subsequent answer, since the topic is cleared up and intentions are clear since the only thing you still do is brag around, even though I started an actual dialogue about what could be improved. This behaviour is not welcome and is not tolerated, you're still welcome to participate, but in a way that is not just profiling yourself and in a way that is constructive.

Thanks for understanding.