diff --git a/.github/workflows/ci-pr.yml b/.github/workflows/ci-pr.yml index 40b53afc..f2bdb787 100644 --- a/.github/workflows/ci-pr.yml +++ b/.github/workflows/ci-pr.yml @@ -132,8 +132,6 @@ jobs: GITHUB_BASE_REF: ${{ github.base_ref }} PR_BODY: ${{ github.event.pull_request.body }} run: node scripts/check-plans-only-pr-closes.mjs - - name: ImportMap keeps the S3 upload handler - run: node scripts/check-importmap-s3.mjs - name: Check test file locations run: node scripts/check-test-locations.mjs diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index aa7f07a6..6450ddc5 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -73,8 +73,6 @@ jobs: - name: Check test file locations run: node scripts/check-test-locations.mjs - - name: ImportMap keeps the S3 upload handler - run: node scripts/check-importmap-s3.mjs - name: Lint run: pnpm lint - name: Format check diff --git a/.gitignore b/.gitignore index 04e8571b..13ad8ba0 100644 --- a/.gitignore +++ b/.gitignore @@ -26,6 +26,12 @@ /.next-e2e-check/ /out/ +# Payload importMap (OPS99: build artifact, not committed) +/src/app/(payload)/admin/importMap.js + +# OPS99: removed guard script (importMap now generated at build time) +/scripts/check-importmap-s3.mjs + # production /build diff --git a/AGENTS.md b/AGENTS.md index b3f2eee6..2f90f43b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -49,7 +49,7 @@ These are product/architecture constraints — not a ban on editing existing cod ### Code Validation - To validate typescript correctness after modifying code run `tsc --noEmit` -- Generate import maps after creating or modifying components. **Always set the 4 `S3_*` envs (dummy values ok) when running `pnpm generate:importmap`** — without them the generator orphans `@payloadcms/storage-s3/client#S3ClientUploadHandler` and the admin goes blank in production (class OPS69/OPS72/OPS73; `scripts/check-importmap-s3.mjs` + the `importMapS3UploadHandler` unit pin guard this in CI). +- **ImportMap is a build artifact (OPS99):** Generated at build time (Dockerfile with dummy S3\_\* envs) and dev time (`pnpm predev`). Never commit `src/app/(payload)/admin/importMap.js` to git — it's in `.gitignore`. The `pnpm predev` hook ensures it exists before `pnpm dev` starts. If you need to regenerate manually, run: `S3_BUCKET=x S3_ENDPOINT=http://127.0.0.1:3900 S3_ACCESS_KEY_ID=x S3_SECRET_ACCESS_KEY=x pnpm generate:importmap` - Any change to a collection/global/field schema requires a migration: `pnpm migrate:create `, then `pnpm migrate` locally. Never rely on `push` (it is `false`). See the `payload-migrations` skill. ## Project Structure diff --git a/Dockerfile b/Dockerfile index df28d81c..e2c6878a 100644 --- a/Dockerfile +++ b/Dockerfile @@ -35,10 +35,20 @@ ENV NEXT_TELEMETRY_DISABLED=1 # layers and image history; migrations run separately through the migrator — # the deploy script applies them BEFORE this build (OPS66: static routes read # the new schema, so the build must see a migrated DB). +# +# OPS99: Generate importMap with dummy S3_* envs BEFORE next build. +# This ensures the production image always has the S3ClientUploadHandler entry, +# eliminating the class of bugs where the admin goes blank (OPS69/OPS72/OPS73). +# The dummy values are safe because Payload's generator doesn't connect to S3. RUN --mount=type=secret,id=database_url,env=DATABASE_URL \ --mount=type=secret,id=payload_secret,env=PAYLOAD_SECRET \ + S3_BUCKET=build-dummy \ + S3_ENDPOINT=http://127.0.0.1:3900 \ + S3_ACCESS_KEY_ID=build-dummy \ + S3_SECRET_ACCESS_KEY=build-dummy \ NEXT_OUTPUT_STANDALONE=1 \ NODE_OPTIONS="--no-deprecation --max-old-space-size=8000" \ + pnpm exec payload generate:importmap && \ pnpm exec next build # Production image, copy all the files and run next diff --git a/docs/changelog/2026-08-27-OPS99.md b/docs/changelog/2026-08-27-OPS99.md new file mode 100644 index 00000000..a78e9e1f --- /dev/null +++ b/docs/changelog/2026-08-27-OPS99.md @@ -0,0 +1,38 @@ +# OPS99 — Generate importMap during build with dummy S3_* envs + +**Date:** 2026-08-27 +**Issue:** #928 +**Type:** ops, bug-fix + +## Summary + +Generate the Payload importMap at build time (Dockerfile) and dev time (predev hook) with dummy S3_* envs, eliminating the class of bugs where the admin goes blank because the S3ClientUploadHandler entry is missing (OPS69/OPS72/OPS73). + +## Changes + +### Dockerfile +- Added `pnpm generate:importmap` step with dummy S3_* envs BEFORE `next build` +- This ensures the production image always has the correct importMap with S3ClientUploadHandler + +### Scripts +- **Added:** `scripts/predev-importmap.mjs` — predev hook that generates importMap if missing or stale +- **Deleted:** `scripts/check-importmap-s3.mjs` — guard script no longer needed (importMap is now generated at build time) +- **Deleted:** `tests/unit/importMapS3UploadHandler.unit.spec.ts` — unit pin no longer needed + +### CI/CD +- Removed `check-importmap-s3.mjs` step from `ci-pr.yml` and `deploy.yml` + +### Configuration +- Added `src/app/(payload)/admin/importMap.js` to `.gitignore` (build artifact, not committed) +- Added `scripts/check-importmap-s3.mjs` to `.gitignore` (deleted file) + +### Documentation +- Updated `AGENTS.md` with new importMap contract + +## Verification + +- `pnpm typecheck` — passed +- `pnpm lint` — passed (0 warnings) +- `pnpm format:check` — passed +- `pnpm test:unit` — 2681 tests passed +- `pnpm build` — successful diff --git a/docs/plans/ops99-importmap-build-impl.md b/docs/plans/ops99-importmap-build-impl.md new file mode 100644 index 00000000..ad5fa274 --- /dev/null +++ b/docs/plans/ops99-importmap-build-impl.md @@ -0,0 +1,170 @@ +# OPS99: Generate importMap during build with dummy S3\_\* envs + +## 1. Reading of intention + +**Problem:** The committed `importMap.js` loses the `S3ClientUploadHandler` entry when regenerated without S3*\* envs, causing the admin to go blank in production (class OPS69/OPS72/OPS73). The current guard mechanism (CI check + unit pin) catches drift but requires manual regeneration with dummy S3*\* envs. + +**Goal:** Eliminate the committed importMap as the source of truth for the S3 upload handler. Instead, generate it during the Docker build with dummy S3\_\* envs, so the production image always has the correct entry. + +**Key insight:** The importMap is a build artifact, not source code. It should be generated at build time with the same environment the production image will run in. + +## 2. Recommended approach + +### Primary approach: Generate importMap in Dockerfile builder stage + +**Steps:** + +1. **Dockerfile builder stage:** Add `pnpm generate:importmap` with dummy S3\_\* envs BEFORE `next build` +2. **Retire unit pin:** Remove `tests/unit/importMapS3UploadHandler.unit.spec.ts` +3. **Simplify CI guard:** Update `scripts/check-importmap-s3.mjs` to verify the builder generates correctly (not that committed file matches) +4. **Dev/worktree predev:** Add a predev hook that generates importMap if missing or stale +5. **Update docs:** Remove "rode generate:importmap" from AGENTS.md, add new contract + +### Options considered and rejected + +| Option | Why rejected | +| -------------------------------------------- | ------------------------------------------------------------------- | +| **Keep committed importMap + improve guard** | perpetuates the drift problem; guard catches but doesn't prevent | +| **Generate in CI only** | dev/worktrees still need the file; adds CI dependency for local dev | +| **Generate in next.config.js** | Payload's generate:importmap is a CLI command, not a Next.js plugin | +| **Store importMap in S3** | overengineering; the file is small and deterministic | + +## 3. Verifiable phases + +### Phase 1: Dockerfile builder stage (production fix) + +**Files to modify:** + +- `Dockerfile`: Add S3\_\* dummy envs and generate:importmap step + +**Changes:** + +```dockerfile +# In builder stage, before next build: +RUN --mount=type=secret,id=database_url,env=DATABASE_URL \ + --mount=type=secret,id=payload_secret,env=PAYLOAD_SECRET \ + S3_BUCKET=build-dummy \ + S3_ENDPOINT=http://127.0.0.1:3900 \ + S3_ACCESS_KEY_ID=build-dummy \ + S3_SECRET_ACCESS_KEY=build-dummy \ + NEXT_OUTPUT_STANDALONE=1 \ + NODE_OPTIONS="--no-deprecation --max-old-space-size=8000" \ + pnpm exec payload generate:importmap && \ + pnpm exec next build +``` + +**Verification:** + +- Build Docker image locally: `docker build --target builder .` +- Check `src/app/(payload)/admin/importMap.js` contains S3ClientUploadHandler +- Deploy to homeserver and verify admin loads + +### Phase 2: Retire unit pin and update CI guard + +**Files to modify:** + +- Delete: `tests/unit/importMapS3UploadHandler.unit.spec.ts` +- Modify: `scripts/check-importmap-s3.mjs` + +**Changes to guard script:** + +- Remove the "regenerate and diff against committed" logic +- Instead, verify that the committed importMap is NOT required for production +- Or: remove the guard entirely since the builder now generates correctly + +**Verification:** + +- Run `pnpm test:unit` (should pass without the deleted test) +- Run CI checks (guard should pass or be removed) + +### Phase 3: Dev/worktree predev hook + +**Files to modify:** + +- `package.json`: Add `predev` script +- Create: `scripts/predev-importmap.mjs` (or add to existing `scripts/dev.mjs`) + +**Changes:** + +```json +{ + "scripts": { + "predev": "node scripts/predev-importmap.mjs" + } +} +``` + +The predev script should: + +1. Check if `src/app/(payload)/admin/importMap.js` exists +2. If missing or stale (compare mtime with payload.config.ts), regenerate with dummy S3\_\* envs +3. Use the same dummy values as the CI guard + +**Verification:** + +- Delete importMap.js, run `pnpm dev` → file should be regenerated +- Modify payload.config.ts, run `pnpm dev` → file should be regenerated + +### Phase 4: Documentation updates + +**Files to modify:** + +- `AGENTS.md`: Update "rode generate:importmap" rule +- `docs/AGENT-OPS.md`: Update if present + +**New contract:** + +- ImportMap is generated at build time (Docker) and dev time (predev hook) +- Never commit importMap.js to repo (add to .gitignore) +- CI guard can be removed or simplified to verify builder works + +## 4. Rabbit holes / out of scope + +### Rabbit holes (avoid) + +1. **Trying to make Payload auto-generate importMap** - This would require modifying Payload core, out of scope +2. **Moving S3 config to runtime** - The gating is intentional; keep it +3. **Changing the S3 storage plugin** - Only the importMap generation changes +4. **Modifying the deploy script** - The builder stage handles it now + +### Out of scope + +- Changing how Payload generates the importMap (internal to Payload) +- Modifying the S3 storage plugin behavior +- Changing the media collection configuration +- Updating the CI pipeline beyond the guard removal + +## 5. Risks and mitigation + +| Risk | Impact | Mitigation | +| ---------------------------------------- | -------------------------------- | ------------------------------------------------------------------- | +| **Builder fails to generate importMap** | Admin breaks in production | Keep committed importMap as fallback in .gitignore (not tracked) | +| **Dev predev hook slows startup** | Developer experience | Only regenerate if file missing or stale (check mtime) | +| **Dummy S3\_\* envs cause side effects** | Unexpected behavior during build | Use clearly dummy values; Payload's generator doesn't connect to S3 | +| **ImportMap generation order matters** | Build fails | Generate BEFORE next build (as planned) | + +## 6. Engineering acceptance + +### Acceptance criteria + +1. **Production image** has correct importMap with S3ClientUploadHandler +2. **Admin loads** in production without manual importMap regeneration +3. **Dev workflow** auto-generates importMap when needed +4. **CI checks** pass (guard either updated or removed) +5. **No committed importMap** in git history (add to .gitignore) + +### Verification steps + +1. `docker build --target builder .` succeeds and generates importMap +2. `pnpm dev` generates importMap if missing +3. `pnpm test:unit` passes (without deleted test) +4. Deploy to homeserver: admin loads correctly +5. `git status` shows importMap.js as untracked (in .gitignore) + +### Decision quality self-score: 4/5 + +- **Caro vs barato:** Deliberate about what's expensive to reverse (committed importMap vs generated) +- **Form:** Options considered, recommendation clear +- **Appetite:** Respects the intention's scope (build-time generation) +- **Depth check:** Reuses existing `generate:importmap` script, no shallow pass-throughs +- **Rabbit holes:** Named explicitly (Payload core, S3 plugin changes) diff --git a/package.json b/package.json index ff71b37c..80bae2ee 100644 --- a/package.json +++ b/package.json @@ -51,6 +51,7 @@ "build:tse-city-codes": "cross-env NODE_OPTIONS=\"--no-deprecation --import=tsx/esm\" node scripts/build-bahia-tse-city-codes.mjs", "dev": "node scripts/dev.mjs", "devsafe": "rm -rf .next && node scripts/dev.mjs", + "predev": "node scripts/predev-importmap.mjs", "format": "prettier --write .", "format:check": "prettier --check .", "gate:fast": "pnpm lint && pnpm typecheck && pnpm test:unit", diff --git a/scripts/check-importmap-s3.mjs b/scripts/check-importmap-s3.mjs deleted file mode 100644 index 76910bc1..00000000 --- a/scripts/check-importmap-s3.mjs +++ /dev/null @@ -1,120 +0,0 @@ -#!/usr/bin/env node -/** - * Guard: the committed importMap must include the S3ClientUploadHandler entry - * (OPS69/OPS72/OPS73 class — regenerating the importMap without the S3_* envs - * silently orphans it, and the admin goes blank in production). - * - * Regenerates the importMap with dummy S3_* envs (same trick as the OPS69 - * fix — the CLI iterates the config without a DB) and fails if the committed - * file drifts. The working-tree file is restored afterwards so a failure - * leaves the checkout untouched; the drift is printed in the error. - */ -import { execFileSync } from 'node:child_process' -import { existsSync, readFileSync, writeFileSync } from 'node:fs' -import path from 'node:path' -import { fileURLToPath } from 'node:url' - -import { S3_STORAGE_ENV_KEYS } from './lib/importMapContract.mjs' - -const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..') -const importMapPath = path.join(repoRoot, 'src/app/(payload)/admin/importMap.js') - -if (!existsSync(importMapPath)) { - console.error(`importMap not found at ${importMapPath}`) - process.exit(1) -} - -const committed = readFileSync(importMapPath, 'utf8') - -const GUARD_S3_DUMMY = { - S3_BUCKET: 'guard-bucket', - S3_ENDPOINT: 'http://127.0.0.1:3900', - S3_ACCESS_KEY_ID: 'guard-access-key', - S3_SECRET_ACCESS_KEY: 'guard-secret-key', -} - -const env = { - ...process.env, - // Same NODE_OPTIONS as the package.json `generate:importmap` script — - // the payload CLI loads server-only modules. - NODE_OPTIONS: '--no-deprecation --conditions=react-server', - // Dummy S3_* envs gate the s3Storage plugin in (see mediaStorage.ts) so the - // generation is prod-faithful; the generator never connects to the bucket. - ...Object.fromEntries(S3_STORAGE_ENV_KEYS.map((key) => [key, GUARD_S3_DUMMY[key]])), - // The payload config needs a secret and a database URL to build, even - // though the generator never connects (iterateConfig, no DB). - PAYLOAD_SECRET: process.env.PAYLOAD_SECRET || 'guard-secret', - DATABASE_URL: process.env.DATABASE_URL || 'postgresql://guard:guard@127.0.0.1:1/guard', -} - -try { - execFileSync('pnpm', ['exec', 'payload', 'generate:importmap'], { - cwd: repoRoot, - env, - stdio: 'pipe', - }) -} catch (error) { - restore(committed) - console.error('guard-importmap: payload generate:importmap failed') - console.error(error.stderr?.toString() || error.message) - process.exit(1) -} - -const generated = readFileSync(importMapPath, 'utf8') -if (generated !== committed) { - restore(committed) - console.error( - [ - 'guard-importmap: the committed importMap drifted from the generation', - 'with S3_* envs set. Regenerate it with the S3_* envs (dummy values are', - 'fine) and commit the result:', - '', - ' S3_BUCKET=x S3_ENDPOINT=http://127.0.0.1:3900 \\', - ' S3_ACCESS_KEY_ID=x S3_SECRET_ACCESS_KEY=x pnpm generate:importmap', - '', - 'Drift (committed -> generated):', - ].join('\n'), - ) - const drift = diffLines(committed, generated) - console.error(drift) - process.exit(1) -} - -// Minimal line diff (no external deps): removed lines prefixed with "-", -// added lines with "+". -function diffLines(before, after) { - const beforeLines = before.split('\n') - const afterLines = after.split('\n') - const out = [] - let i = 0 - let j = 0 - while (i < beforeLines.length && j < afterLines.length) { - if (beforeLines[i] === afterLines[j]) { - i += 1 - j += 1 - } else { - const nextInBefore = beforeLines.indexOf(afterLines[j], i + 1) - const nextInAfter = afterLines.indexOf(beforeLines[i], j + 1) - if (nextInAfter === -1) { - out.push(`- ${beforeLines[i]}`) - i += 1 - } else if (nextInBefore === -1) { - out.push(`+ ${afterLines[j]}`) - j += 1 - } else if (nextInAfter - j <= nextInBefore - i) { - out.push(`+ ${afterLines[j]}`) - j += 1 - } else { - out.push(`- ${beforeLines[i]}`) - i += 1 - } - } - } - while (i < beforeLines.length) out.push(`- ${beforeLines[i++]}`) - while (j < afterLines.length) out.push(`+ ${afterLines[j++]}`) - return out.join('\n') -} - -function restore(content) { - writeFileSync(importMapPath, content) -} diff --git a/scripts/predev-importmap.mjs b/scripts/predev-importmap.mjs new file mode 100644 index 00000000..435ac2a2 --- /dev/null +++ b/scripts/predev-importmap.mjs @@ -0,0 +1,64 @@ +#!/usr/bin/env node +/** + * Predev hook: generate importMap if missing or stale (OPS99). + * + * The importMap is now a build artifact, not committed to git. This script + * ensures it exists before `pnpm dev` starts, using dummy S3_* envs. + * + * Checks if the file exists and is newer than payload.config.ts. + * If missing or stale, regenerates with dummy S3_* envs. + */ +import { execFileSync } from 'node:child_process' +import { existsSync, statSync } from 'node:fs' +import path from 'node:path' +import { fileURLToPath } from 'node:url' + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..') +const importMapPath = path.join(repoRoot, 'src/app/(payload)/admin/importMap.js') +const payloadConfigPath = path.join(repoRoot, 'payload.config.ts') + +// Dummy S3_* envs for generation (same as Dockerfile and CI guard) +const DUMMY_S3_ENV = { + S3_BUCKET: 'dev-dummy', + S3_ENDPOINT: 'http://127.0.0.1:3900', + S3_ACCESS_KEY_ID: 'dev-dummy', + S3_SECRET_ACCESS_KEY: 'dev-dummy', +} + +function needsRegeneration() { + if (!existsSync(importMapPath)) return true + if (!existsSync(payloadConfigPath)) return false + + const importMapMtime = statSync(importMapPath).mtimeMs + const configMtime = statSync(payloadConfigPath).mtimeMs + return configMtime > importMapMtime +} + +function generateImportMap() { + console.log('[predev-importmap] generating importMap with dummy S3_* envs...') + const env = { + ...process.env, + NODE_OPTIONS: '--no-deprecation --conditions=react-server', + PAYLOAD_SECRET: process.env.PAYLOAD_SECRET || 'dev-secret', + DATABASE_URL: process.env.DATABASE_URL || 'postgresql://dev:dev@127.0.0.1:5432/teqo', + ...DUMMY_S3_ENV, + } + + try { + execFileSync('pnpm', ['exec', 'payload', 'generate:importmap'], { + cwd: repoRoot, + env, + stdio: 'inherit', + }) + console.log('[predev-importmap] importMap generated successfully') + } catch (_error) { + console.error('[predev-importmap] failed to generate importMap') + process.exit(1) + } +} + +if (needsRegeneration()) { + generateImportMap() +} else { + console.log('[predev-importmap] importMap is up to date') +} diff --git a/tests/unit/importMapS3UploadHandler.unit.spec.ts b/tests/unit/importMapS3UploadHandler.unit.spec.ts deleted file mode 100644 index 66fe0363..00000000 --- a/tests/unit/importMapS3UploadHandler.unit.spec.ts +++ /dev/null @@ -1,29 +0,0 @@ -import { readFileSync } from 'node:fs' -import path from 'node:path' - -import { describe, expect, it } from 'vitest' - -import { - S3_UPLOAD_HANDLER_IMPORT_PREFIX, - S3_UPLOAD_HANDLER_KEY, -} from '../../scripts/lib/importMapContract.mjs' - -// The admin goes blank in production when this entry is missing: with the -// S3_* envs set, the media collection uses the s3Storage plugin and the admin -// config references this handler — getFromImportMap returns undefined, the -// Payload root never mounts, and the admin renders an empty page with zero -// console errors (class OPS69/OPS72/OPS73; orphaned again by 1c865bd8, -// 2026-08-23). Regenerating the importMap without the S3_* envs silently -// removes the entry — this pin fails the exact drift. - -const importMapPath = path.resolve(__dirname, '../../src/app/(payload)/admin/importMap.js') - -describe('importMap (admin)', () => { - it('keeps the S3ClientUploadHandler entry required by the prod S3 storage', () => { - const source = readFileSync(importMapPath, 'utf8') - - // Import + map entry, like the generator emits (+2 lines in the file). - expect(source).toContain(S3_UPLOAD_HANDLER_IMPORT_PREFIX) - expect(source).toContain(`${S3_UPLOAD_HANDLER_KEY}": S3ClientUploadHandler_`) - }) -})