From d9916de6adb0a882bb40bac2d34b6ea6f9807a35 Mon Sep 17 00:00:00 2001 From: Seenu K Date: Wed, 12 Aug 2026 14:55:15 +0530 Subject: [PATCH] Give the scaffolder's foreign-file edits one whitespace rule Three functions append a block to a file `flutter create` produced. Each normalised newlines itself, with two different answers, so the desugaring block got a blank line before it and the other two did not: in a generated project the two Gradle blocks landed flush against each other. `appendBlock` now owns that and nothing else, with the block constants carrying no leading or trailing newline of their own, and the scaffold test pins the separation rather than only the contents. A comment beside the release-signing block claimed it still prepends `import java.util.Properties`. That edit was removed when AGP 9 broke it, so the comment described the one thing the surrounding code exists to avoid. MAINTAINERS.md gains the doctrine these three follow: append never insert, why each block is recognised by a content probe on its own payload, and why FlutterFire's `// START:`/`// END:` markers are deliberately not copied. Those markers are attribution rather than machinery, never read back by flutterfire_cli itself, and an append needs no anchor. The one thing that would earn a marker pair is a block whose content must be found and replaced on a later run, which belongs to an upgrade command that does not exist yet; the section records the shape to use when it does. It also records that configuration values are assigned into declared slots instead, and the JSON-versus-JSONC rule governing how. Co-Authored-By: Claude Opus 5 --- .changeset/append-never-insert.md | 30 +++++ MAINTAINERS.md | 105 ++++++++++++++++++ packages/create-eigen-game/src/index.ts | 56 +++++++--- .../create-eigen-game/test/scaffold.spec.ts | 15 +++ 4 files changed, 189 insertions(+), 17 deletions(-) create mode 100644 .changeset/append-never-insert.md diff --git a/.changeset/append-never-insert.md b/.changeset/append-never-insert.md new file mode 100644 index 0000000..468ff82 --- /dev/null +++ b/.changeset/append-never-insert.md @@ -0,0 +1,30 @@ +--- +"create-eigen-game": patch +--- + +Give the three edits into files the scaffolder does not own one shared whitespace +rule, and write down the doctrine they follow. + +`enableAndroidCoreLibraryDesugaring`, `enableAndroidReleaseSigning` and +`configureLauncherIconsAndSplash` each append a block to a file `flutter create` +produced. All three normalised newlines themselves, with two different answers, +so the desugaring block got a blank line before it and the other two did not: in +a generated project the two Gradle blocks ended up flush against each other. +`appendBlock` now owns that and nothing else, one blank line before and one +newline after, with the block constants carrying neither, and the scaffold test +pins the separation rather than only the contents. + +No behavioural change beyond the whitespace of generated `build.gradle.kts` and +`pubspec.yaml`. A comment claiming the release-signing block still prepends a +Kotlin `import` was also stale, describing an edit removed when it broke under +AGP 9, and said the opposite of the rule it sat next to. + +MAINTAINERS.md gains **Editing files the scaffolder does not own**: append never +insert, why every block is recognised by a content probe on its own payload, and +why FlutterFire's `// START:`/`// END:` marker comments are deliberately not +copied. Those markers are attribution rather than machinery, never read back by +`flutterfire_cli` itself, and an append needs no anchor; the one condition that +would earn a marker pair is a block whose content must be found and replaced on a +later run, which is an upgrade command's problem and not one that exists yet. It +also records that configuration values are assigned into declared slots instead, +and the JSON-versus-JSONC rule that governs how. diff --git a/MAINTAINERS.md b/MAINTAINERS.md index 62af239..97591fa 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -382,6 +382,111 @@ line-crossing release. defeat the purpose: a real resolution against pub.dev is the only thing that makes the pin mean anything. +### Editing files the scaffolder does not own + +Most of what `create-eigen-game` writes, it owns: four template trees copied +whole and token-substituted by `renderTree`. Three edits are different. They +modify files `flutter create` produced, in a directory `flutterfire configure` +will edit next, and they have one rule. + +> **Append, never insert.** + +| Function | File | Identified by | +|---|---|---| +| `enableAndroidCoreLibraryDesugaring` | `android/app/build.gradle.kts` | `isCoreLibraryDesugaringEnabled` | +| `enableAndroidReleaseSigning` | same file | `releaseKeyProperties` | +| `configureLauncherIconsAndSplash` | `app/pubspec.yaml` | `flutter_launcher_icons:` | + +An append cannot compete for position with whatever Flutter or FlutterFire +writes above it. Both Android edits are second top-level `android { }` blocks, +which is legal because Gradle merges repeated extension-configuration blocks in +one file; the pubspec edit is two top-level YAML keys. A second top-level +`plugins { }` block is *not* legal, one per script, which is why the Crashlytics +plugin registration is left to `flutterfire configure` and not attempted here. + +The rule is not a preference. The one edit that ever needed a position, +prepending `import java.util.Properties` because Kotlin requires imports before +every other top-level declaration, was broken by an AGP upgrade, and was fixed by +removing the need for it rather than by chasing the new layout. Parsing +`key.properties` with the Kotlin stdlib needs no import. Reach for a positional +edit and you have signed up for that maintenance again. + +Each block is recognised by **a content probe on its own payload**, chosen to be +a string only that block contains. `includes("signingConfigs")` would not do: +Flutter's own template already contains it, via +`signingConfig = signingConfigs.getByName("debug")`, so the guard would +short-circuit forever and nothing would ever be appended. Two of the three also +assert on the *shape* of what they found and throw: an Android application module +must be there, and a half-written desugaring configuration is an error rather +than something to append alongside. + +`appendBlock` owns the whitespace, and only the whitespace: one blank line before +the block, one newline after, block constants carrying neither. It is one +function because it was three, with two different answers, so the second Gradle +block landed flush against the first in every generated project. + +#### Why FlutterFire's marker comments are not copied + +`flutterfire configure` brackets its Gradle insertions: + +```kotlin +// START: FlutterFire Configuration +id("com.google.gms.google-services") version("4.4.4") apply false +// END: FlutterFire Configuration +``` + +**Those markers are attribution, not machinery.** In flutterfire_cli 1.4.1 the +two constants appear in ten places and every one is string construction; nothing +ever reads them back. Its idempotency comes from a separate content probe, +`contains(_googleServicesPluginClassPath)`, which is the same mechanism the table +above uses. FlutterFire needs a *regex anchor* because it must inject into the +single permitted `plugins { }` block, and the comment rides along; an append +needs no anchor, so a marker would add text without adding capability. + +Copying them would also be actively misleading in one respect: FlutterFire uses +one marker text for all its insertions, so it could not replace them selectively +even if it read them. + +**A marker pair earns its place under exactly one condition: the block's content +changes between runs and the old copy must be found and replaced.** That is what +a content probe cannot do. `includes("releaseKeyProperties")` answers "is mine +here?" but never "where does mine end?", so it can only skip, never update, and +`desugar_jdk_libs:2.1.4` is frozen into every project already generated. + +That condition arrives with an upgrade command, not before. When it does, add +markers as a genuinely parsed region: **one pair per block**, versioned in the +marker text so a future writer knows what it is replacing, and guarded on +finding *exactly one* properly nested pair. Anything else, and they are +decoration. + +Never put markers in `templates/`. Those files are wholly owned and rendered +whole, and a marker there would promise a tool that revisits them. + +#### Configuration values are assigned, not appended + +Engine configuration is a separate case from the three edits above, and does not +use them. `FIREBASE_PROJECT_ID`, `WEB_APP_ORIGIN`, `GOOGLE_WEB_CLIENT_ID`, +`API_BASE_URL` and the VAPID key all have declared, empty slots in +`templates/worker/wrangler.jsonc` and `templates/app-overlay/app-config.json`. +Filling a slot beats appending a block: the key is visible in the template before +anything runs, the wrangler schema knows about it, and an absent or ambiguous +slot is a refusal rather than a guess. + +`configure_firebase` in eigen-flutter does that filling, at scaffold time and on +every later re-run, so there is one implementation rather than two that drift. +Its rule, in `lib/src/codegen/firebase_link.dart`: + +| File | Method | Why | +|---|---|---| +| `app-config.json` | decode, merge, re-encode | Plain JSON; nothing to preserve | +| `wrangler.jsonc` | rewrite the one assignment in place | Comments. Re-encoding would delete every one, and Cloudflare recommends JSONC for new projects | + +The surgical path's safety is the **match count, not the pattern**: a key found +anywhere other than exactly once, commented out, duplicated, or nested under some +other object, means this is not the file it knows how to edit, so nothing is +written and the caller reports it. Guessing which of two matches was meant is the +only outcome worth avoiding. + ## Downstream documentation `eigen-web` commits generated copies of: diff --git a/packages/create-eigen-game/src/index.ts b/packages/create-eigen-game/src/index.ts index a9863d4..fccf9e7 100644 --- a/packages/create-eigen-game/src/index.ts +++ b/packages/create-eigen-game/src/index.ts @@ -373,6 +373,32 @@ function packageCommand(manager: PackageManager, operation: "install" | "contrac return [manager, ["run", operation]]; } +/** + * Appends `block` to a file this scaffolder does not own, as its own paragraph. + * + * **Append, never insert.** That is the rule the three callers below follow, and + * it is what keeps them from competing with `flutter create` and `flutterfire + * configure` for position in the same files. A pure append cannot collide with + * whatever those tools write above it; the one edit here that ever needed a + * position, prepending a Kotlin `import`, was broken by an AGP upgrade and has + * since been rewritten away. See MAINTAINERS.md for the whole doctrine, including + * why FlutterFire's `// START:`/`// END:` marker comments are deliberately not + * copied. + * + * `contents` is passed in rather than read here because every caller has already + * read the file to decide whether its block is present, and two of them assert + * on the shape of what they found. This owns the whitespace and nothing else. + * + * That is worth one function because it was previously written three times with + * two different answers, so the desugaring block got a blank line before it and + * the other two did not, and the two Gradle blocks ended up flush against each + * other in every generated project. Separation belongs to the append, so the + * block constants carry no leading or trailing newline of their own. + */ +function appendBlock(path: string, contents: string, block: string): void { + appendFileSync(path, `${contents.endsWith("\n") ? "" : "\n"}\n${block}\n`); +} + const androidDesugaring = `// flutter_local_notifications requires desugaring in the application module. // A library plugin cannot enable this compiler setting transitively. android { @@ -397,7 +423,7 @@ function enableAndroidCoreLibraryDesugaring(appRoot: string): void { if (settingExists || dependencyExists) { throw new Error(`Flutter created an incomplete Android core library desugaring configuration: ${gradlePath}`); } - appendFileSync(gradlePath, `${gradle.endsWith("\n") ? "\n" : "\n\n"}${androidDesugaring}\n`); + appendBlock(gradlePath, gradle, androidDesugaring); } // A second top-level `android { }` block is valid Gradle Kotlin DSL, since Gradle @@ -407,10 +433,12 @@ function enableAndroidCoreLibraryDesugaring(appRoot: string): void { // script), which is why the Crashlytics Gradle plugin registration, // `flutterfire configure`'s own territory, is deliberately left alone here. // -// `Properties` needs a real `import`, prepended separately below. A -// fully-qualified `java.util.Properties()` reference here was tried first, -// to avoid needing to touch the top of the file at all, but fails to -// resolve under AGP 9's Kotlin DSL script compilation +// Parsed with the Kotlin stdlib rather than `java.util.Properties`, which is +// what keeps this appendable. `Properties` would need a real `import`, and +// Kotlin requires imports before every other top-level declaration, so using it +// meant also reaching into the very start of a file this scaffolder does not +// own. A fully-qualified `java.util.Properties()` reference was tried to avoid +// that, and does not resolve under AGP 9's Kotlin DSL script compilation // (`flutter create`'s current default): "Unresolved reference 'util'". // Confirmed by actually building a scaffolded project, not just reading it. const androidReleaseSigning = `val releaseKeyProperties: Map = @@ -455,13 +483,9 @@ function enableAndroidReleaseSigning(appRoot: string): void { // so that check would always short-circuit and never append anything. if (gradle.includes("releaseKeyProperties")) return; // A pure append. This previously also PREPENDED `import java.util.Properties`, - // because Kotlin requires imports before every other top-level declaration, - // which meant reaching into the very start of a file this scaffolder does - // not own, the most position-dependent edit here and the one already broken - // once by an AGP upgrade. Parsing `key.properties` with the Kotlin stdlib - // needs no import, so the edit is now append-only and cannot collide with - // whatever Flutter or `flutterfire configure` writes above it. - appendFileSync(gradlePath, `${gradle.endsWith("\n") ? "" : "\n"}${androidReleaseSigning}\n`); + // the most position-dependent edit here and the one already broken once by an + // AGP upgrade; see the block above for what replaced it. + appendBlock(gradlePath, gradle, androidReleaseSigning); } // `flutter_launcher_icons`/`flutter_native_splash` read these as plain @@ -475,8 +499,7 @@ function enableAndroidReleaseSigning(appRoot: string): void { // reversed, light-on-dark variant, so an ink background is what makes it // legible. Rebranding a game means replacing the four PNGs *and* these // colours together. See https://eigeninteractive.com/docs/ship-it/branding. -const launcherIconsAndSplashConfig = ` -flutter_launcher_icons: +const launcherIconsAndSplashConfig = `flutter_launcher_icons: android: true web: generate: true @@ -499,14 +522,13 @@ flutter_native_splash: image_dark: assets/icon/splash_dark.png icon_background_color: "#F4F1EA" icon_background_color_dark: "#1B1E24" - web: true -`; + web: true`; function configureLauncherIconsAndSplash(appRoot: string): void { const pubspecPath = resolve(appRoot, "pubspec.yaml"); const pubspec = readFileSync(pubspecPath, "utf8"); if (pubspec.includes("flutter_launcher_icons:")) return; - appendFileSync(pubspecPath, `${pubspec.endsWith("\n") ? "" : "\n"}${launcherIconsAndSplashConfig}`); + appendBlock(pubspecPath, pubspec, launcherIconsAndSplashConfig); } export interface AddContinuousIntegrationOptions { diff --git a/packages/create-eigen-game/test/scaffold.spec.ts b/packages/create-eigen-game/test/scaffold.spec.ts index ec20934..42a9857 100644 --- a/packages/create-eigen-game/test/scaffold.spec.ts +++ b/packages/create-eigen-game/test/scaffold.spec.ts @@ -224,12 +224,27 @@ describe("scaffoldGame", () => { expect(androidGradle).toContain('coreLibraryDesugaring("com.android.tools:desugar_jdk_libs:2.1.4")'); expect(androidGradle).toContain("signingConfigs"); expect(androidGradle).toContain("proguardFiles("); + // Each appended block is separated from what it follows by exactly one blank + // line, including from the other block this scaffolder appends to the same + // file. Newline normalisation used to be written per call site with two + // different answers, so the second Gradle block landed flush against the + // first in every generated project. This is generated code someone reads. + expect(androidGradle).toContain("}\n\n// flutter_local_notifications requires desugaring"); + expect(androidGradle).toContain("}\n\nval releaseKeyProperties"); + expect(androidGradle).not.toMatch(/\n\n\n/); + expect(androidGradle.endsWith("}\n")).toBe(true); expect(readFileSync(resolve(root, "app/fastlane/Appfile"), "utf8")).toContain('package_name("games.example.chess")'); const appPubspec = readFileSync(resolve(root, "app/pubspec.yaml"), "utf8"); expect(appPubspec).toContain("flutter_launcher_icons:"); expect(appPubspec).toContain("flutter_native_splash:"); + // Same separation rule as the Gradle appends above, and here it is load + // bearing rather than cosmetic: these are top-level YAML keys, so a block + // that landed without its own line break would be read as part of whatever + // `flutter create` wrote last. + expect(appPubspec).toContain("sdk: flutter\n\nflutter_launcher_icons:"); + expect(appPubspec.endsWith(" web: true\n")).toBe(true); // `scaffoldGame` only creates `--platforms android,web`, so `ios`/`macos` // keys would make `flutter_launcher_icons` error on a platform directory // that doesn't exist.