alternative suggestion:
other than file paths, "import" and "require" can also be objects, containing "types" and "default".
could leverage those declarations here, which might make dist-fix.js obsolete?

(edit - forgot to link to) docs:

• https://nodejs.org/api/packages.html#conditional-exports
• https://nodejs.org/api/packages.html#community-conditions-definitions
• https://www.typescriptlang.org/docs/handbook/modules/reference.html#example-explicit-types-condition

Interesting! 🤔

I think we'd still want to alter how tsup outputs and have a folder for each still. What are the downsides?

have a folder for each still

intuitively I'd agree. (read: me wanting to organize and put different things into different boxes)

at the same time, that requirement may be obsolete?

• these days .mjs file extensions seem widely supported.
• the specs allow for arbitrary paths via "exports" & co-located type def files with matching extensions. wanting an "esm"/"cjs" folder seems like custom build decision that doesn't really impact lib users(?)
• I'd trust tools like tsup to know better than my own intuition; there's likely a reason outputting separate folders is non-default and moved behind the aptly named --legacy-output flag. ¯\_(ツ)_/¯

What are the downsides?

omitting --legacy-output would generate ESM files using .mjs file extensions. I haven't worked with any bundler/environment in some time that does not understand that extension. theoretically some (archaic?) environment might not understand .mjs, but for that to be relevant they'd also need to somehow explicitely want to load the .mjs file over the co-located CJS .js file.

(Hypothesis: Could be that this was more relevant at a time when ESM was still competing with CJS and IIFE. Back then, I don't think we had extensions to differentiate between those, or they weren't as well established yet; and we certainly did not have "exports" in package.json back then.)

(Personally I'd even be in favor of leaving CJS in the dust. But I understand that, for now, it may still be controversial forcing that decision as lib author.)

OK, let's ditch legacy output flag. See if anyone complains. Worst case, we'll revert back.

See if anyone complains.

nice - Test-In-Prod is my middle name 👋 changes pushed! (edit: re-pushed; missed to update the cjs imports in the knexfile)

I've made sure to keep the /// <reference .../> injection (now also widened to the CSJ files).
not fully sure how to handle the chunk files (or they need any handling) for deno, so I've omitted them for now. if problematic, can also config tsup to not output chunks I believe.

while I was at it, I've also moved the dist-fix script to ESM in a separate comit - hope that's cool? no real reason other than cLeAnUp. happy to revert if CJS is preferred.

Why was this change necessary?

not necessary per-se, and independent of the core of this PR. it's a separate commit before the actual one that splits the bundle; figured if I'm already here, might as well clean up sth on my way out.

I noticed that TS was not happy with the KyselyFsMigrationSource type assignment here, complaining that:

let migrationSource: KyselyFsMigrationSource
                     ~~~~~~~~~~~~~~~~~~~~~~~
'KyselyFsMigrationSource' refers to a value, but is being used as a type here. Did you mean 'typeof KyselyFsMigrationSource'?

However, switching to typeof KyselyFsMigrationSource causes even more TS problems.

Using InstanceType<typeof KyselyFsMigrationSource> is more accurate with the later assignment, but then TS still complains about this type not perfectly matching what knex.migrate.rollback expects.

Falling back to knex's own FsMigrations type aligns with the variable assignment, and with with knex itself later expects.

...just one of the cases were I do sympathize with those who hate TS and its type juggling. 😅

I see. Let's go with InstanceType<typeof KyselyFsMigrationSource>. The build step turned the exported class declaration into var x = class.

I see. Let's go with InstanceType.

roger, pushing this. as per my earlier post, this still causes TS complain in two places:

await ctx.knex.migrate.latest({migrationSource})
                               ~~~~~~~~~~~~~~~
Type 'KyselyFsMigrationSource' is missing the following properties from type 'MigrationSource<unknown>': getMigrations, getMigrationNamets(2739)

but these type errors should be safe to ignore.

The build step turned the exported class declaration into var x = class.

can you clarify (if worth it)? the ESM output should not be relevant to the TS warning, and this TS type declaration here should have no impact on any build or test output.

let's add a minimal deno test @ tests/deno to verify we're building this stuff correctly..
see https://github.com/kysely-org/kysely/tree/master/test/deno for reference (just local is fine).

added

disclaimer: never used deno, so very new to both it and this kind of deno testing. 😅 tried to roughly replicate the setup from the kysely repo with a new bare-bones test. not sure if this aligns with what you had in mind though(?)

I also don't think this setup actually tests our TS-only /// <reference ...> injection at all. If I alter the reference, the deno script still completes fine. So this might only somewhat tests if the bundled runtime code is deno-compatible.