Help from @brenoepics

[Shinigami92]

#1222 (comment)

By the way, if you ever need docs for a feature, fix, or PR, feel free to assign me! I’d be happy to help, as well as look into issues or investigate problems. 🙂

@brenoepics if you would like to, there are some tests that are marked with e.g. // TODO @Shinigami92 2024-04-04: Should this throw an error?

node-pg-migrate/test/utils/makeComment.spec.ts

Line 6 in 93e398c

// TODO @Shinigami92 2024-04-04: Should this throw an error?

There are definitely more like this in the test-code-base.

So if you want to work on these things, this would be a great help 🚀
Please make a dedicated PR per function-change. And we also need sometimes to investigate what to really do. Throw error? Remove functionality? Do something else?

---

After we solved many of the todos, I'm planning either for 8.x (>8.0) or 9.0 a pretty option #1122

[brenoepics]

Sounds good! I’ll check out the TODOs and make separate PRs for each or ask if anything needs further discussion before making changes.

[brenoepics]

For some reason, we don't have force-exit used here documented at https://salsita.github.io/node-pg-migrate/cli

actually, isn't it likely unreachable code? I think the IDEs don't mark it because of process.exit(x) and not return but it's the same because we have both in then and in catch.

Maybe it is an workaround to:

node-pg-migrate/bin/node-pg-migrate.ts

Lines 636 to 650 in e37aa21

	const promise =
	action === 'redo'
	? migrationRunner(options('down')).then(() =>
	migrationRunner(options('up', Number.POSITIVE_INFINITY, false))
	)
	: migrationRunner(options(action));
	promise
	.then(() => {
	console.log('Migrations complete!');
	process.exit(0);
	})
	.catch((error: unknown) => {
	console.error(error);
	process.exit(1);
	});

we dont await at L640 nor return promise result, so sometimes promise will do .then or .catch sometimes not that depends on a lot of stuff and force-exit can actually be called?

(if we actually await, everything works well but it takes a few ms or seconds more to run)

I'm imagining something like this

function demoEarlyExit() {
  const promise = new Promise((resolve) => {
    console.log("1. Promise created");
    setTimeout(() => {
      console.log("2. Promise resolved");
      resolve();
    }, 100);
  });

  promise.then(() => {
    console.log("3. Then block");
    process.exit(0);
  });

  console.log("4. Exiting early!");
  process.exit(2); // This exits before the promise can resolve
}

demoEarlyExit();

//RESULT:
//1. Promise created
//4. Exiting early!

I'm working on TODO: /bin vitest coverage, rn I have about 90% coverage but need to check this.

[brenoepics]

@Shinigami92

Just to make sure, I noticed that before jiti support, this setup would throw an error anyway since we load all files in the migrations folder, regardless of whether -j js/ts/... was set. But now that we use jiti, it's likely going to run all migrations, no matter if they're JS or TS (because previously it just didn't run them due to the error, if you used ts-node it would run everything too). So if this is the intended behavior, the -j option isn't needed at all besides for the create command. If it's not intended, we should check for -j and only load migrations matching the chosen type(s).

But keep in mind that using create with -j js -j ts (maybe you might have it already set in package.json script and set again in cli making it an array) will throw an error and create an empty file.

 ts-node node_modules/.bin/node-pg-migrate -j js -j ts --config-file config/test.json --dry-run --tsconfig tsconfig.json create testing

[Error: ENOENT: no such file or directory, open '../node_modules/node-pg-migrate/templates/migration-template.js,ts'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '../node_modules/node-pg-migrate/templates/migration-template.js,ts'

We could make a better error/template checking here.

tl;dr

npm run migrate up also works for typescript so we could at least simplify docs.

[Shinigami92]

You are right. And yes, -j is only intended for the creation suffix. If it is possible we could even change the behavior in the current "next" major and run any migration file in order even when mixed!
So imagine someone has a x.ts, a y.js and a z.sql file in the migrations folder. Then it will run them after each other in x,y,z. Would you agree with that?

[brenoepics]

That makes sense yeah, in the current order we do timestamp so it is fine (actually, jiti is crazy, all the formats are working!?)

> Migrating files:
> - 175355580761_test (this is ts)
> - 175355580762_test (this is cjs)
> - 175355580763_test (this is cts)
> - 175355580764_test (this is mjs)
> - 1753537218125_test (this is js)
> - 1753555807618_test (this is mts)
> - 1753555807619_test (this is sql)

The only thing we don't have is the -j validation to check if the template exists.

[Shinigami92]

@brenoepics I got a bit distracted by other projects, but I remember that we merged #1460 already
Is there anything we want to do before releasing a v9 non-alpha?
Because I don't like to bump the major version number so often if it is possible to omit

[brenoepics]

I think it’s fine, but do you think we need this discussion? I’ve been thinking another issue is that .js and .ts migrations with the same name could overwrite each other when we convert them to .sql. Plus, the migration log doesn’t show the file extension, so we have no clue which is which.

Overall I don't think there is any breaking change to do, so other changes can come later.

[Shinigami92]

Yeah I have the exact same thing in my head and I'm also a bit unsure. IMO if there are fully same named files, ts should win over js and js should win over sql. Or we could even throw an error in such a case 🤔

Exporting an up and down to .sql could be done as a feature in any minor release as it does not break anything.

OK, maybe I will just release the v9 tomorrow and press my thumbs 😆