Skip to content

Add opt-in setting to automatically include fixtures in migrations on change - #269

Draft
benjie wants to merge 4 commits into
mainfrom
watch-fixtures
Draft

benjie wants to merge 4 commits into
mainfrom
watch-fixtures

Conversation

@benjie

@benjie benjie commented Oct 14, 2026

Copy link
Copy Markdown
Member

Untested; just to demonstrate approach

@benjie
benjie added this pull request to stack #267 October 14, 2026 14:13
@xvaara

xvaara commented Oct 14, 2026 •

Copy link
Copy Markdown

From claude opus:

From claude opus (updated for #269 at ee03d5f; #262 has since dropped its baseline file):

Short version: #269's approach is better and should be the base. #262 should not land as it is. Its tests and a few fixes are worth porting as small follow-ups; its file-writing design and watch cache are not.

Where #269 is better

  • It never writes to the developer's files. Add opt-in automatic fixture blocks #262 rewrites the current migration while reading it, in watch, status and commit. That is where most of its +540 lines of source go: compare-and-swap writes with retries, symlink checks, generated file numbering, a block grammar, and an error when files keep changing during a refresh. Several bugs we fixed during review came from exactly that machinery. In Add opt-in automatic fixture blocks #262, status also edits the working tree, which a status command shouldn't do.
  • Less than half the code for the same core behaviour (+253 lines of source).
  • Placement. "Fixtures first, include one manually to run it later" is one simple rule and matches the include example in the README. Add opt-in automatic fixture blocks #262 appends to the end and lets you move the block. That suits fixtures that depend on new schema, but a manual include covers that case in both designs, and neither placement avoids every silent ordering problem.
  • Uncommit and failed commits are handled by stripping one marked section in writeCurrentMigration, which is simpler than Add opt-in automatic fixture blocks #262 round-tripping its generated blocks.
  • The setting's docs warn against auto fixtures for stateful objects (indexes, constraints, materialized views). Add opt-in automatic fixture blocks #262 never says this.
  • No cache to trust. Add opt-in automatic fixture blocks #262 reuses compiled fixtures in watch and recompiles only those named by watcher events. That took a run with nothing changed from ~600 ms to ~70 ms with 10,000 fixtures. But with 1,000 fixtures a full rescan is only ~55 ms, and the cache needs a watcher-event protocol, a 3-second recent-write guard and a mode-switch reset to stay correct. For most projects that isn't worth it.

What #262 could still contribute

In order of value:

  1. Tests. Add opt-in setting to automatically include fixtures in migrations on change #269 has none. Add opt-in automatic fixture blocks #262 has ~940 lines. Its lifecycle tests port directly: run, commit, uncommit and recommit with fixtures; comment-only changes; nested changes; deleted fixtures; following history when a migration is removed; adopting the setting in an existing project. The block-specific tests don't.
  2. The header fix (issue 1 below).
  3. One run per fixture: take nested and manual includes from the compiled migration (issues 2 and 3).
  4. A failed-commit fix for main, separate from this PR. Since fix: preserve migration headers after failed commit #273 a failed commit writes the compiled migration back, so manual includes come back expanded and the next read rejects the --! Included markers. Restoring the directives (uncompileIncludes(contents)) fixes that.
  5. Docs.
    • Adoption: enabling the setting includes every fixture once. Re-running 1,000 each of functions, views, policies, comments and grants took ~0.4 s locally, so the real cost is the locks that migration holds until commit.
    • Execution order: when a fixture needs a manual include to run later, and how watch can hide ordering mistakes.

What #262 has that #269 lacks, weighed honestly:

Issues in #269 at ee03d5f

From reading the code; I haven't run it. Fixed since my first comment: stripping the auto section on uncommit and failed commits, detecting manual includes regardless of spacing, and skipping hidden folders.

  1. Headers. The auto section is prepended above the header lines, so commit no longer recognises --! Message:. In current.sql mode, --! no-transaction is no longer the first line, so the migration runs in a transaction. The strip in writeCurrentMigration also removes everything before --!endAuto, so on uncommit it drops the headers that serializeMigration writes first, such as a -m message. Inserting the section after the headers, and stripping only the section, fixes all three.
  2. Nested fixtures run twice. When a.sql includes b.sql and b.sql changes, both are prepended. Only the outer one is needed.
  3. Some manual includes aren't recognised. ./x.sql (the TODO) and fixtures reached through another manually included fixture are prepended and run again. The --! Included markers of the compiled current migration give the complete list.
  4. Empty or comment-only new fixtures are included (undefined !== hash).

Both PRs share one limitation: pg-minify 1.6.3 doesn't understand dollar quoting. Text after -- inside a $tag$…$tag$ string is treated as a comment, so such edits go undetected, and some valid SQL (for example comment on function f() is $$Don't…$$;) makes it throw.

Next steps

I'm happy to close #262 and instead open small PRs on top of #269 (or push to your branch, whichever you prefer) for items 1–3 and the docs, plus the failed-commit fix against main. I won't port the generated blocks or the watch cache unless someone runs into the problems they solve.

Two questions:

  • Should a changed --! no-transaction fixture fail the commit rather than log a warning? A warning during commit is easy to miss, and the change then never ships.

@benjie
benjie force-pushed the watch-fixtures branch 2 times, most recently from 89720f6 to 03d89cf Compare October 14, 2026 14:59
Base automatically changed from postprocess-once to main October 14, 2026 15:02
@benjie
benjie removed this pull request from stack #267 October 14, 2026 15:02
@benjie
benjie force-pushed the watch-fixtures branch 2 times, most recently from ee03d5f to 404164c Compare October 15, 2026 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants