Minor bundle regression: Un-treeshakable "FsMigrations" import from knex

[echocrow]

Issue

Version 0.2.0 added support for migrations! In that update, a new index-imported top-level file, src/fs-migration-source.ts, was added, which imports the following from knex:

import {FsMigrations} from 'knex/lib/migrations/migrate/sources/fs-migrations.js'

Unfortunately the knex package does not declare itself as "sideEffects": false the way kysely-knex does. As such, this import is not treeshaken away when not needed - even when KyselyFsMigrationSource is treeshaken away.

This issue manifests itself further downstream: Packages that import kysely-knex will end up bundling that particular knex file, which itself includes various imports from path, os etc. (potentially problematic in certain runtimes) and further bloats bundle outputs.

Repro

I can spin up a proper repro if requested, but basic, minimal example looks like this:

// src/my-file.ts
import {KyselyKnexDialect} from 'kysely-knex'

export function foo() {
  new KyselyKnexDialect({} as any)
}

Worst case this file, when bundled, will already include the entire fs-migrations.js file knex.
Best case, when declaring knex as external dependency, the dist output may look something like this (exact output depends on the bundler, target, etc.):

// dist/my-file.js
import { FsMigrations } from "knex/lib/migrations/migrate/sources/fs-migrations.js";
var KyselyKnexConnection = class {
  #connection;
  #knex;
  // ...
};

function foo() {
  new KyselyKnexDialect({});
}
export {
  foo
};

However, the next package that utilizes this, without declaring knex (or specifically knex/lib/migrations/migrate/sources/fs-migrations.js) as external, will in turn bundle said knex file into its dist output.

[echocrow]

There may be better ways to handle this, but two potential solutions off the top of my head:

• A: somehow mark knex/lib/migrations/migrate/sources/fs-migrations.js as tree-shakable? Unsure if this can be done in this package without requiring upstream changes to the knex package.
• B: perhaps move the migrations feature out of the top-level index exports and into a separate subpath export? This would bypass the issue, but also represent a breaking change: Migration features would then need to be imported e.g. via import {KyselyFsMigrationSource} from 'kysely-knex/migrate'.

[igalklebanov]

Great catch!

I'm leaning towards B, kysely-knex/migrations.

[echocrow]

would a PR for option B be welcome? code change should be minimal and I can likely get to it later this week

[igalklebanov]

Sure.