Slonik 30

[simoneb]

Slonik 30 has been released with the main breaking change being that createPool is now async.

Upon a quick spike to migrate this codebase I eventually came to the conclusion that it's a considerable amount of work as there are many places "tainted" by this change, including places where the lack of availability of top level await won't make it easy to propagate this change.

[ripecosta]

I've done this to be able to use slonik v30, in case it helps...

import type { DatabasePool } from 'slonik';
import { createPool } from 'slonik';

const slonik = createPool(connUrl);

const migrator = slonik.then(
  s =>
    new SlonikMigrator({
      slonik: s,
      migrationsPath: path.join(__dirname, '../../migrations'),
      migrationTableName: 'migration',
      logger: console,
    }),
);

if (require.main === module) {
  migrator
    .then(m => m.runAsCLI())
    .catch(err => {
      console.error('failed to migrate', err);
    });
}

It can probably be done for the other tools too, which would make v0.11.3 compatible without any changes, right?

EDIT: Alternatively, something similar to the above code can be placed within the migrator. But it would require changed the exports to a Promise<SlonikMigrator> instead.

[mmkal]

Here's a hack I'm going to try at work. If it goes well maybe it could be used here (or merged into slonik):

import {once} from 'lodash'
import {createPool, DatabasePool, ClientConfigurationInput} from 'slonik'
import {createClientConfiguration} from 'slonik/dist/src/factories/createClientConfiguration'

type AsyncPoolMethod = {
  [K in keyof DatabasePool]: DatabasePool[K] extends (...args: any[]) => Promise<any> ? K : never
}[keyof DatabasePool]
type NonAsyncPoolMethodKey = Exclude<keyof DatabasePool, AsyncPoolMethod>

export type LazyPool = Omit<DatabasePool, NonAsyncPoolMethodKey> & {
  configuration: DatabasePool['configuration']
  getPoolState: () => Promise<ReturnType<DatabasePool['getPoolState']>>
}

export const createLazyPool = (uri: string, config: ClientConfigurationInput): LazyPool => {
  const memoizedCreatePool = once(createPool)
  const getPool = async () => memoizedCreatePool(uri, config)
  const syncify =
    <Name extends AsyncPoolMethod>(name: Name): DatabasePool[Name] =>
    async (...args) => {
      const pool = await getPool()
      // @ts-expect-error union type isn't callable but this will work at runtime
      return pool[name](...args) // eslint-disable-line @typescript-eslint/no-unsafe-return
    }
  return {
    any: syncify('any'),
    anyFirst: syncify('anyFirst'),
    configuration: createClientConfiguration(config),
    connect: syncify('connect'),
    copyFromBinary: syncify('copyFromBinary'),
    end: syncify('end'),
    exists: syncify('exists'),
    getPoolState: async () => getPool().then(p => p.getPoolState()),
    many: syncify('many'),
    manyFirst: syncify('manyFirst'),
    maybeOne: syncify('maybeOne'),
    maybeOneFirst: syncify('maybeOneFirst'),
    one: syncify('one'),
    oneFirst: syncify('oneFirst'),
    query: syncify('query'),
    stream: syncify('stream'),
    transaction: syncify('transaction'),
  }
}

You can then use a LazyPool almost identically to a DatabasePool. The only functional difference is getPoolState returns a promise of the pool state instead of being synchronous.

otherwise everything else can be used the same way. And every call will pass through a wrapper function that ideally wouldn't need to exist 🙃 .

FYI @gajus v30 was a very significant breaking change. It's not just a superficial API/code-style change. It fundamentally changes the way users have to think about createPool. Previously, a pool was an ephemeral concept. Calling createPool could safely be done from anywhere, because there are no side-effects in the constructor. Now, users have to think really carefully about where they call it. In my opinion it should have been in a different function which called the original createPool internally. Are there discussions that happen, or is there a medium where users can give feedback before changes like this go in?

Next time you're thinking about making a huge change like this, it'd be great to have a discussion about it, so we can talk through how it affects libraries like this (especially if you're using it yourself!)

[simoneb]

@mmkal yeah that's what I was trying to convey in the original message. This is a major change for this library, doable nonetheless, just a lot of work. On the other hand, I think @gajus has all the right to change the public API of his library in a major semver release.

[mmkal]

The above workaround worked in our CI but caused strange problems when deployed to our Lambdas which connect to RDS via RDS-Proxy. So will have to try again and debug soon, but not ready to do it confidently yet.

[samuliasmala]

Is the challenge here the other slonik-tools packages? For me the @slonik/migrator works nicely using slonik v30 and IIFE:

// migrate.js
const {SlonikMigrator} = require('@slonik/migrator')
const {createPool} = require('slonik')

(async () => {
  // in an existing slonik project, this would usually be setup in another module
  const slonik = await createPool(process.env.POSTGRES_CONNECTION_STRING)

  const migrator = new SlonikMigrator({
    migrationsPath: __dirname + '/migrations',
    migrationTableName: 'migration',
    slonik,
    logger: SlonikMigrator.prettyLogger,
  })

  migrator.runAsCLI()
})();

[szist]

@mmkal bump? the above workaround from samuliasmala seems to work here as well.