Write node-pg-migrate in TypeScript

[Shinigami92]

closes #501

[Shinigami92]

Ok ... first "problem" 😸
We have some templates migration-template.js, migration-template.sql and migration-template.ts
Because they are currently in lib (and now in src) they would get transpiled to js...
We could ignore them with tsconfig exclude or I think this is the more clean way move them into a template folder
Then we could use path.resolve(__dirname, `../templates/migration-template.${suffix}`)

Do you (@dolezel) have a better solution?

[Shinigami92]

Current state

• 32 tests passing
• 13 test failing

Problem

https://github.com/Shinigami92/node-pg-migrate/blob/725ea4a/src/migration.ts#L44

Doesn't work as expected 🤔
Tests failing with Migration is not a constructor

Ref

• https://stackoverflow.com/questions/34384093/how-to-export-constructor-in-a-declare-module
• https://www.typescriptlang.org/docs/handbook/modules.html

---

Cant finish today 😕
Have some meetings and tomorrow on the way by train 😅

[Shinigami92]

All types from index.d.ts are used in src folder ✅

[Shinigami92]

I used module.exports for Migration and db
Now it's working and all tests passes 👍

But I will work and improve more later

[Shinigami92]

The second option in this line is never used 🤔

node-pg-migrate/lib/runner.js

Line 193 in e883e5b

await lock(db, options);

Function definition:

node-pg-migrate/lib/runner.js

Lines 54 to 63 in e883e5b

	const lock = async db => {
	const {
	rows: [lockObtained]
	} = await db.query(
	`select pg_try_advisory_lock(${PG_MIGRATE_LOCK_ID}) as "lockObtained"`
	);
	if (!lockObtained) {
	throw new Error('Another migration is already running');
	}
	};

Should I remove the second parameter? Or add a TODO comment? Or ignore it for the moment and wait on another PR?

[moltar]

Legend!

[dolezel]

@Shinigami92 great 🎉 👏 I hope I will find time tomorrow to look at it.

1. moving templates outside src is probably best
2. if the parameter passed to lock is not used, just drop it :)
3. I do not remember now out of head, but I think that you can define column simply specifying SQL string?

[Shinigami92]

1. Will look into it later
2. done
3. Will look into it later, may have something to do with:
   https://github.com/Shinigami92/node-pg-migrate/blob/typescript/src/utils.ts#L129
   and
   https://github.com/Shinigami92/node-pg-migrate/blob/typescript/src/operations/tables.ts#L89

[Shinigami92]

3. Yes, I checked it and it can be a string

In example if we specify { id: 'id' }, we want to use the shorthand

[dolezel]

Making it consistent with the type and with usage on other places. It is a breaking change. Needs to be mentioned in release notes!

[Shinigami92]

I did not wanted to touch your runtime code
But an improvement could be to use default parameters in functions instead of using destructuring in the body of the function

[dolezel]

Sure, I understand, but I'm afraid that there are some issues which will become apparent once we will switch to TS :/

[Shinigami92]

I think the move to a next branch is very well!

Also I can try to run a migration with my project and look if there are any errors
But I think your tests are already very well

[Shinigami92]

I will try to update this branch in the next few days

[dolezel]

I'm merging it to next branch and will create a testing release. If you will find any issues, please open new PR...

[dolezel]

npm i node-pg-migrate@next

[Shinigami92]

I will do so in some minutes/hours

[moltar]

I have tested in my little (so far) setup with only 2 migrations and 2 tables. And it worked flawlessly.

[Shinigami92]

I tested my project: https://github.com/Shinigami92/star-citizen-trade-market-api

yarn add -DT node-pg-migrate@next # installed ~4.0.0-alpha.0
yarn migrate up
yarn migrate down 3
yarn migrate up

Everything works like a charm

[garth]

Spent some time trying to figure out why I couldn't get this setup working out of the box. Compared my config with the one at https://github.com/Shinigami92/star-citizen-trade-market-api and made some observations which I documenting here just in case someone else also has similar problems.

1. You need to run node-pg-migrate via ts-node
2. Some typescript compiler options are not compatible, for example tsconfig.json from nextjs uses "module": "esnext"

I used the following package script to get my setup working:

"migrate": "ts-node --compiler-options '{\"module\":\"commonjs\"}' node_modules/node-pg-migrate/bin/node-pg-migrate --migration-file-language ts",

First time I've had typescript support in migrations and it's really great!

[timtonk]

Hey guys, how is the testing going? Do you have any approx date when this upgrade will land on master?

[dolezel]

RC candidate published 🎉