chore: monorepo devcontainer support, refactor db adapter and docker start script#16396
chore: monorepo devcontainer support, refactor db adapter and docker start script#16396
Conversation
| "name": "Payload Monorepo", | ||
| "image": "mcr.microsoft.com/devcontainers/base:ubuntu", | ||
| "features": { | ||
| "ghcr.io/payloadcms/devcontainer-features/mise-bootstrap:1": {}, |
There was a problem hiding this comment.
This is new: https://github.com/payloadcms/devcontainer-features
Allows us to share the entire installation / package manager portion of this setup across our other repos without being repetitive.
| // setup.sh auto-detects bind-mount vs volume-clone mode and handles both. | ||
| "postCreateCommand": ".devcontainer/setup.sh", | ||
| // Only the Next.js dev server should be forwarded | ||
| "forwardPorts": [3000], |
There was a problem hiding this comment.
We do not want it to auto-forward all the databases that may be started within the container
| fi | ||
|
|
||
| # Seed .env when absent | ||
| [ -f .env ] || cp .devcontainer/.env.example .env |
There was a problem hiding this comment.
Through this, new clone => open in devcontainer will have an .env file with PAYLOAD_DATABASE=sqlite
| @@ -0,0 +1,17 @@ | |||
| import * as p from '@clack/prompts' | |||
There was a problem hiding this comment.
Functionality-wise same as before, but now a ts script, and uses clack for pretty terminal progress display
| * Resolve the host:port to probe for an adapter, honoring the same URL env | ||
| * vars the adapter would. Returns null for adapters that don't need a probe. | ||
| */ | ||
| function getTarget(adapter: DatabaseAdapterType) { |
There was a problem hiding this comment.
This should also work for users that use a manually installed, external db instead of our docker scripts, as it reads the db env vars just like our generated database adapter would
| } | ||
| return 'mongodb' | ||
|
|
||
| return 'sqlite' |
There was a problem hiding this comment.
NEW: Changed the default db adapter from mongodb to sqlite. That way, this repo can be run on a fresh clone without having to install docker or doing any setup.
| process.env.TURBOPACK = '1' | ||
| } | ||
|
|
||
| await assertDbReachable(getCurrentDatabaseAdapter()) |
There was a problem hiding this comment.
Early-exit if db can't connect with helpful error message, before Next.js runs
| "devsafe": "node ./scripts/delete-recursively.js '**/.next' && pnpm dev", | ||
| "docker:clean": "node ./scripts/docker-clean.js", | ||
| "docker:start": "pnpm docker:clean && docker compose -f test/docker-compose.yml --profile all up -d --wait", | ||
| "docker:clean": "node ./scripts/docker-clean.ts", |
There was a problem hiding this comment.
Monorepo requires a newer node version that supports running ts natively => no need for swc
📦 esbuild Bundle Analysis for payloadThis analysis was generated by esbuild-bundle-analyzer. 🤖
Largest pathsThese visualization shows top 20 largest paths in the bundle.Meta file: packages/next/meta_index.json, Out file: esbuild/index.js
Meta file: packages/payload/meta_index.json, Out file: esbuild/index.js
Meta file: packages/payload/meta_shared.json, Out file: esbuild/exports/shared.js
Meta file: packages/richtext-lexical/meta_client.json, Out file: esbuild/exports/client_optimized/index.js
Meta file: packages/ui/meta_client.json, Out file: esbuild/exports/client_optimized/index.js
Meta file: packages/ui/meta_shared.json, Out file: esbuild/exports/shared_optimized/index.js
DetailsNext to the size is how much the size has increased or decreased compared with the base branch of this PR.
|
There was a problem hiding this comment.
This is great, thank you!
- My main suggestion would be to use
@devcontainers/cliinstead of the VS Code extension, as I mentioned in a comment below. - It is great that we all share a unified environment. Still, I am a bit concerned this might hide bugs that only appear in specific setups like different operating systems or versions of Node or pnpm. I agree the tradeoff is probably worth it, and more testing is likely the only real safeguard. I just wanted to flag it.
- Aside from that, below I copy the review of Opus 4.7 after removing a couple of suggestions that seemed irrelevant to me:
Claude Review
Blockers
1)
docker:cleananddocker:startwill not run on Node 18 / 20 (declared engines)
package.jsonruns them with barenode:"docker:clean": "node ./scripts/docker-clean.ts", "docker:start": "node ./scripts/docker-start.ts",But
engines.nodeis^18.20.2 || >=20.9.0. Native TS stripping (--experimental-strip-types) only exists on Node 22.6+, and the imports use.tsextensions ('./docker-lib.ts','../test/dbAdapters.ts'), which require additional flags even on Node 22+. The rest of the repo uses thepnpm runtswrapper (which loads@swc-node/register). These scripts should do the same:"docker:clean": "pnpm runts ./scripts/docker-clean.ts", "docker:start": "pnpm runts ./scripts/docker-start.ts",Note that
runtsis defined with--no-experimental-strip-types, indicating the project intentionally avoids native stripping. Keep it consistent.2) Inconsistent default DB adapter
The PR changes the default from
mongodbtosqlite:
test/dbAdapters.ts:export const getCurrentDatabaseAdapter = (): DatabaseAdapterType => { const dbAdapter = process.env.PAYLOAD_DATABASE as DatabaseAdapterType | undefined if (dbAdapter && Object.keys(dbAdapters).includes(dbAdapter)) { return dbAdapter } return 'sqlite' }
test/vitest.setup.ts:if (!process.env.PAYLOAD_DATABASE) { process.env.PAYLOAD_DATABASE = 'sqlite' }But
test/initDevAndTest.tswas left on'mongodb':const dbAdapter: DatabaseAdapterType = (process.env.PAYLOAD_DATABASE as DatabaseAdapterType) || 'mongodb'Pick one and apply it everywhere. Also call out the change in the PR description and update
CLAUDE.md/CONTRIBUTING.mdaccordingly (currentCLAUDE.mdstill says "Default database is MongoDB").Pre-merge cleanup
3)
pnpm-lock.yamlcontains unrelated changesA fair amount of churn around
next@16.2.3(@babel/core@7.29.0)(...)peer-dep injection, unrelated to@clack/prompts. Likely a re-resolve from a stale lockfile. Rebase ontomainand regenerate the lock so the diff stays scoped to the actual change.4)
package.json:@clack/promptsis alphabetically out of order"@swc-node/register": "1.11.1", "@swc/cli": "0.7.9", "@swc/core": "1.15.3", "@clack/prompts": "1.2.0", "@types/fs-extra": "^11.0.2",Should sit before
@swc-node/register.5)
.devcontainer/.dockerignoreis probably not honoredDocker reads
.dockerignorefrom the build context root, not from.devcontainer/. Sincedevcontainer.jsonusesimage:directly (no custom build context), this file does not take effect unless the config is restructured to usedockerfile+context. Either move it to the repo root or drop it.Suggestions
6)
assertDbReachable2000 ms timeoutconst result = await tcpPing(target.host, target.port, 2000)In CI with loaded runners or cold-starting containers, 2 s can be tight. Locally it is great UX. Consider bumping to 5–10 s in CI or adding a short retry with backoff.
7) Misleading probe for
cosmosdb/documentdb/firestoreThese share the
MONGOblock, so they probelocalhost:27018even though they actually point to cloud services. If the user has not setMONGODB_URL, the probe can:
- Pass deceptively when a local MongoDB happens to be running, or
- Fail with a message suggesting
pnpm docker:start mongodb, which does not applySkip the probe for these adapters unless
MONGODB_URLis set.8)
scripts/docker-start.tshardcodes storage ports{ name: 'storage', label: 'LocalStack, Azurite, fake-GCS, Vercel Blob', hint: '4443, 3100, 4566, 10000', },For postgres/mongo the hint is computed from
dbAdapters, but storage is hardcoded. If compose ports change it drifts. Consider adding a port map for non-DB services indbAdapters.tsordocker-lib.ts.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Done, I got rid of the
Done
Lockfile is up-to-date, so doesn't matter
Done
Done, deleted it
Done, bumped to 10s when in ci
We test against these db adapters using the same local mongodb but with compatibility options set, so I don't think the probe doesn't apply here
Don't think it matters whether the hardcoded ports are in docker-start.ts or docker-lib.ts since they are only read in one place. This is simpler
Yea, only way to address this is to run CI against different systems. This change is only optional though, people can still use their own machine / manually installed or external databases |
3 participants
Devcontainers
Adds dev container configuration, so contributors can open the repo in VS Code without setting up Node, pnpm, or Docker on their own machine. Works for both "Reopen in Container" (your local clone bind-mounted in) and "Clone Repository in Container Volume" (a fresh clone inside an isolated Docker volume, useful when running multiple parallel sessions against the same repo without them stepping on each other).
test/generateDatabaseAdapter.tswas renamed totest/dbAdapters.tsand is the source of truth for anything db-adapter-related in one place: the source templates the codegen writes out, and the host/port/env-var defaults per adapter, which the newassertDbReachable.tsfunction uses to probe services.This should enable running multiple agents conflict-free
DB Connection probe + docker:start script improvements
assertDbReachableis now run withinpnpm dev. If the db connection fails, you now get immediate feedback through a helpful error, instead of having to wait for Next.js to compile and then be thrown off by a cryptic db seed error:pnpm docker:startis now an interactive picker. You can still pass profile names as args (pnpm docker:start postgres mongodb) to skip the prompt:These changes encourages contributors not to start every single service we have available (uses around 4gb of ram), and only start the database service that you're using => much less memory usage.