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.