Split migrations into separte bundle #29

echocrow · 2024-04-20T21:07:50Z

Fixes #26

src/migrations.ts

echocrow

left some code comments for point-of-interest & suggestions

additionally:
I believe it's somewhat common practice to have the src folder resemble the exports structure. could achieve this by moving all files except index.ts and migrations.ts into a new subdirectory, e.g. src/lib. optional and skipped for now, but figured worth calling out, now that there's more than one export.
(edit: above note was made redundant with the switch to the src/migrations subdir)

echocrow · 2024-04-20T21:14:45Z

package.json

+    },
+    "./migrations": {
+      "import": "./dist/esm/migrations.js",
+      "require": "./dist/cjs/migrations.js"


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.

tests/nodejs/migrations.test.ts

igalklebanov · 2024-04-22T08:24:12Z

tests/nodejs/migrations.test.ts

@@ -12,7 +13,7 @@ import {
 for (const dialect of SUPPORTED_DIALECTS) {
  describe(`KyselyFsMigrationSource: ${dialect}`, () => {
    let ctx: TestContext
-    let migrationSource: KyselyFsMigrationSource
+    let migrationSource: FsMigrations


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.

scripts/dist-fix.js

see kysely-org#29 (comment)

echocrow · 2024-04-26T08:56:32Z

looks like node 18 is failing while node 20 is fine. import.meta.dirname is a node 20+ thing, which is likely what's causing the node 18 tests to fail during build. (somewhat of a false positive for tests; tests should still pass on node 18, but build requring 20+ is now getting in the way.)
probably not worth making the test pipeline more complex, i'll just revert my last ESM-switch commit and keep the dist-fix build script in CJS.

edit: last commit removed; dist-fix is back to CSJ. also rebased onto main while I was at it.

tsup emits `*.d.mts` type def files that were left unused; output seems to be identical to the CJS type def equivalent

see kysely-org#29 (comment)

scripts/dist-fix.js

igalklebanov · 2024-04-27T09:08:28Z

scripts/dist-fix.js

-        await unlink(distEsmFilePath)
+        const dtsFilePath = filename.replace(/\.(m?)js$/, '.d.$1ts')
+        const denoFriendlyJsFileContents =
+          `/// <reference types="${dtsFilePath}" />\n` +


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.

igalklebanov · 2024-04-29T19:50:32Z

Let's add https://www.npmjs.com/package/@arethetypeswrong/cli to package.json#scripts (attw --pack . this works for me in a different project) and CI. It verifies we're exporting CJS/ESM correctly.

echocrow · 2024-05-02T09:48:37Z

Let's add https://www.npmjs.com/package/@arethetypeswrong/cli to package.json#scripts (attw --pack . this works for me in a different project) and CI. It verifies we're exporting CJS/ESM correctly.

neato! heard of the package a while ago; looking forward to seeing it in action myself.
I think integrating it for this package might a bit overkill (small package & exports; we're mostly testing tsup's output?), but it's also only one more package/script/command.

I should have some time this weekend to get back to this 🤞

igalklebanov · 2024-05-02T10:08:48Z

Let's add npmjs.com/package/@arethetypeswrong/cli to package.json#scripts (attw --pack . this works for me in a different project) and CI. It verifies we're exporting CJS/ESM correctly.

neato! heard of the package a while ago; looking forward to seeing it in action myself. I think integrating it for this package might a bit overkill (small package & exports; we're mostly testing tsup's output?), but it's also only one more package/script/command.

I should have some time this weekend to get back to this 🤞

we're testing the package.json configuration is aligned with tsup, and nothing's missing.

echocrow · 2024-05-02T10:53:28Z

we're testing the package.json configuration is aligned with tsup, and nothing's missing.

hence the "mostly" in my post 😅 had I feeling I'd get called out on this

subpath export currently fails for node10

echocrow · 2024-05-04T17:57:31Z

@igalklebanov attw added to scripts! available via test:types.

I was hoping I could add this to the combined test script. however, attw validation currently fails for node10 (but passes everywhere else), and thus errors with exit code 1.
node10 resolution fails because package.json#exports support was added later. The attw docs call out this issue
Unsure if you care about node10 support. I skimmed other examples that illustrate setups for subpath export, but couldn't get them to pass attw's node10 check either. (I also could not find a way to ignore node10 without blanked-ignoring resolution failures.)

thoughts?

igalklebanov · 2024-06-12T07:56:15Z

Sorry for delay. Life and other efforts.

Check out this project for reference. non-legacy tsup with 2 entry points, a single export condition (I don't think it matters), attw is passing in prepublishOnly and a GitHub workflow.

echocrow · 2024-07-16T10:12:11Z

Sorry for delay. Life and other efforts.

no need to apologize! but same here ATM. not sure when I'll get back to this; hopefully in the next few weeks.

cheers for the reference! I'll check check out that project first thing when looping back to this

igalklebanov reviewed Apr 20, 2024

View reviewed changes

src/migrations.ts Outdated Show resolved Hide resolved

echocrow commented Apr 20, 2024

View reviewed changes

igalklebanov reviewed Apr 22, 2024

View reviewed changes

tests/nodejs/migrations.test.ts Outdated Show resolved Hide resolved

igalklebanov reviewed Apr 22, 2024

View reviewed changes

scripts/dist-fix.js Outdated Show resolved Hide resolved

echocrow added a commit to echocrow/kysely-knex-split-migrations that referenced this pull request Apr 22, 2024

ditch legacy dist output

a17eb63

see kysely-org#29 (comment)

echocrow changed the title ~~Split migrations in separte bundle~~ Split migrations into separte bundle Apr 22, 2024

echocrow added a commit to echocrow/kysely-knex-split-migrations that referenced this pull request Apr 22, 2024

ditch legacy dist output

437bd4d

see kysely-org#29 (comment)

echocrow force-pushed the main branch from 23246ea to fbc6f7a Compare April 22, 2024 21:55

echocrow added 7 commits April 26, 2024 11:09

use .d.mts dist file for ESM type definitions

e114c64

tsup emits `*.d.mts` type def files that were left unused; output seems to be identical to the CJS type def equivalent

bypass false-positive type error

c4b72b8

split migrations into separate export

7c7e30d

move migrations src into subdir

477319a

keep test file src specific

94cc5c9

prefer more specific TS type def in test file

93f2b15

ditch legacy dist output

796c5e7

see kysely-org#29 (comment)

echocrow force-pushed the main branch from fbc6f7a to 796c5e7 Compare April 26, 2024 09:10

igalklebanov reviewed Apr 27, 2024

View reviewed changes

scripts/dist-fix.js Outdated Show resolved Hide resolved

igalklebanov reviewed Apr 27, 2024

View reviewed changes

scripts/dist-fix.js Outdated Show resolved Hide resolved

igalklebanov reviewed Apr 27, 2024

View reviewed changes

echocrow added 2 commits May 4, 2024 19:24

clean up dist fix

e8d2aa9

add script to test export types

bd5fbad

subpath export currently fails for node10

add minimal deno tests

58d404d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split migrations into separte bundle #29

Split migrations into separte bundle #29

echocrow commented Apr 20, 2024

echocrow left a comment •

edited

Loading

echocrow Apr 20, 2024 •

edited

Loading

igalklebanov Apr 22, 2024

echocrow Apr 22, 2024

echocrow Apr 22, 2024

igalklebanov Apr 22, 2024 •

edited

Loading

echocrow Apr 22, 2024 •

edited

Loading

igalklebanov Apr 22, 2024

echocrow Apr 22, 2024

igalklebanov Apr 22, 2024 •

edited

Loading

echocrow Apr 22, 2024

echocrow commented Apr 26, 2024 •

edited

Loading

igalklebanov Apr 27, 2024

echocrow May 4, 2024

igalklebanov commented Apr 29, 2024 •

edited

Loading

echocrow commented May 2, 2024

igalklebanov commented May 2, 2024 •

edited

Loading

echocrow commented May 2, 2024

echocrow commented May 4, 2024

igalklebanov commented Jun 12, 2024 •

edited

Loading

echocrow commented Jul 16, 2024

Split migrations into separte bundle #29

Are you sure you want to change the base?

Split migrations into separte bundle #29

Conversation

echocrow commented Apr 20, 2024

echocrow left a comment • edited Loading

Choose a reason for hiding this comment

echocrow Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igalklebanov Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

echocrow Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igalklebanov Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echocrow commented Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igalklebanov commented Apr 29, 2024 • edited Loading

echocrow commented May 2, 2024

igalklebanov commented May 2, 2024 • edited Loading

echocrow commented May 2, 2024

echocrow commented May 4, 2024

igalklebanov commented Jun 12, 2024 • edited Loading

echocrow commented Jul 16, 2024

echocrow left a comment •

edited

Loading

echocrow Apr 20, 2024 •

edited

Loading

igalklebanov Apr 22, 2024 •

edited

Loading

echocrow Apr 22, 2024 •

edited

Loading

igalklebanov Apr 22, 2024 •

edited

Loading

echocrow commented Apr 26, 2024 •

edited

Loading

igalklebanov commented Apr 29, 2024 •

edited

Loading

igalklebanov commented May 2, 2024 •

edited

Loading

igalklebanov commented Jun 12, 2024 •

edited

Loading