Skip to content

.architecture-roadmap.md

Latest commit

 

History

History
192 lines (136 loc) · 15.7 KB

.architecture-roadmap.md

File metadata and controls

192 lines (136 loc) · 15.7 KB
Raw

Crescent architecture roadmap (multi-phase extraction)

Status: Draft, codex-reviewed (findings incorporated below). Scope: Continue the sub-package extraction pattern started by rounds 1–2 (router/ in 95b76f3, cookie/ in 31bf3c8) to shrink the root package and give related code discoverable homes. Invariant across all phases: every phase is a single commit, moon check && moon test must stay green, pkg.generated.mbti is regenerated, and each phase ships as its own PR.

Codex review findings applied

  • [P1] Phase 2 API description was wrong. The real API is zero-arg HttpResponse::ok() + chained .body(...), NOT HttpResponse::ok(body). Combined with the method-coherence rule (public methods on a type can only be defined in the type's owning package; private local methods on foreign types are allowed), all the HttpResponse::ok/created/bad_request/not_found/error/redirect/... factory methods must stay at root. Phase 2 therefore shrinks to extracting the Responder trait + impls + html()/text() helpers only.
  • [P1] Phase 3 (static.mbt merge) was mis-scoped. Root static.mbt owns the abstraction (StaticAssetMeta, ServeStaticProvider, StaticResolvedResponder, App::static_assets); static_file/ is only one provider against that trait. This is not legacy wrapper cleanup. Phase 3 is deferred until a bigger rethink of the static-files abstraction layer.
  • [P1] Phase 4 prerequisite was stale. Current main already has App::ws with private ws_static_routes / ws_dynamic_routes and serve_async.mbt reads those fields directly. Option A (pub(r) field) / Option B (register method) is not the real issue. Phase 4 keeps the existing App::ws method and private storage at root; the extraction moves only the types and runtime.
  • [P2] Phase order reshuffled: middleware → responder → websocket → [defer static].
  • [P2] MoonBit re-exports via pub using actually exist. The blocker for cross-package re-exports from root is the import cycle, not language syntax. No behavioural change, but the wording in .architecture-plan.md should be updated on next edit.
  • [P2] security_wbtest.mbt blackbox rewrite is sound — confirmed by codex. Proceed.
  • [P2] websocket_async_native_wbtest.mbt is genuinely whitebox (inspects native_ws_hubs, NativeWebSocketConnection, private runtime IDs). It will move as a wbtest, not be renamed.
  • [P2] for "test" imports of a child package that imports root should work (blackbox tests are a downstream compilation, not part of root's DAG). Keep the "inline custom middleware" fallback handy in case moon check disagrees.
  • [P2] TooManyRequests will resolve unqualified in Phase 1 rate_limit.mbt via type-directed inference from the status_code~ : StatusCode field.

Why (non-negotiable goals)

  1. Shrink the root package surface — the root .mbt file list currently has ~30 source files spanning server runtime, middleware, response helpers, static assets, fetch, websocket, test client, and typed handlers. A new reader can't tell what's "core" vs "adjacent".
  2. Preserve method coherence — MoonBit disallows defining methods on a type from a different package than the type's definition. Any cluster that owns a method on App or Event must either stay at root or leave its method accessor at root (see cors/ precedent, confirmed again for Phase 1's Event::request_id).
  3. Preserve blackbox-test-first preference — per stored feedback, avoid _wbtest.mbt unless priv access is genuinely needed. Extractions are a natural moment to rewrite wbtests as blackbox.
  4. No source-level BC shims. MoonBit has no fnalias/pub fn = @other.fn syntax, and round 2's attempt to re-export from root hit an import cycle (@middleware → root → @middleware). Every phase that moves a public function is a breaking API change, and downstream migration is "update moon.pkg imports + rename call sites". That's the round-1/round-2 precedent.

Phase table (ordered)

Phase Sub-package LOC moved (src+test) Coupling Plan doc
1 middleware/ rate_limit + request_id + security, ~400 LOC low .architecture-plan.md (already codex-reviewed once, findings above)
2 responder/ only the Responder trait + impls + html()/text() from responder.mbt, ~390 LOC (src+test). response_helpers.mbt and redirect.mbt STAY at root — they are HttpResponse::* methods that coherence pins to root. low §Phase 2 below
3 websocket/ websocket.mbt + websocket_async.mbt + wbtest, ~2500 LOC. App::ws stays at root; only types + runtime move. medium §Phase 3 below
4 (deferred) static_file abstraction split needs a rethink — root currently owns the trait and static_file/ owns one provider implementation. Not a simple merge. medium deferred, not in this roadmap

Deferred / explicitly not doing in this roadmap:

  • testing/ for TestClient — circular dep (TestClient → App; root tests → TestClient). Idiomatic MoonBit testing keeps test harness with the type it dispatches on. Leave at root.
  • typed_handlers/ for App::get/post/put/... convenience methods and wrap_error_handler — method coherence rule binds them to root. Moving the helper functions alone while leaving the methods at root makes the API incoherent.
  • server/ for serve_async.mbt and NativeServeOptions — serve is the heart of the framework and calls into every other root type; extracting it creates cycles with middleware and requires exposing App internals as pub. Not worth the churn.
  • fetch/ consolidation — already complete. The fetch/ sub-package owns the high-level API, FFI enums (FetchCredentials, FetchMode), and the low-level native binding (fetch/fetch_impl.native.mbt). No root-level fetch.mbt / fetch.native.mbt remain.

Phase 1 — middleware/ extraction

See .architecture-plan.md for the detailed plan. That document has already been through one round of codex review and has the review feedback incorporated. Open questions at the bottom of that file remain; they should be re-answered by codex before implementation starts.

Carry-over items from that plan: confirm enum-variant cross-package resolution for StatusCode::TooManyRequests, confirm for "test" root import of a sub-package that itself imports root, rewrite security_wbtest.mbt as blackbox.

Phase 2 — responder/ sub-package (revised after codex review)

Goal

Extract only the Responder trait, its seven impls, and the html() / text() helpers from responder.mbt into a new bobzhang/crescent/responder sub-package. The HttpResponse factory methods (ok, created, bad_request, not_found, error, unauthorized, forbidden, internal_server_error, no_content, redirect, redirect_307, redirect_308, redirect_temporary) stay at root because MoonBit's method-coherence rule pins public methods to the type's owning package.

What moves

  • responder.mbt → responder/responder.mbt — trait + impls for String, StringView, Bytes, Json, ToJson, HttpResponse, HttpRequest + html() / text() helper free functions
  • responder_test.mbt → responder/responder_test.mbt

What stays at root (codex [P1])

  • response_helpers.mbt — every function in this file is a HttpResponse::xxx static method (factory constructors). Method coherence blocks moving them.
  • redirect.mbt — same reason; HttpResponse::redirect, redirect_307, redirect_308, redirect_temporary are all methods.
  • response_helpers_test.mbt, redirect_test.mbt — stay with their source files.

Why this is still worth a phase

Even just moving Responder declutters root (removes ~184 LOC of trait definition + impls) and gives the trait a discoverable home. Downstream type annotations shift from &Responder (unqualified, imported from @crescent) to &@responder.Responder, which documents the dependency better.

Trait coherence check

MoonBit's orphan rule: an impl Trait for Type block is allowed when either the trait or the type is locally defined. Moving the trait definition into responder/ makes it "trait-local", so impl Responder for HttpResponse (a root type) is legal in the new package. Codex confirmed.

What responder/moon.pkg needs to import

  • bobzhang/crescent for HttpResponse, HttpRequest, StatusCode variants that appear in impls
  • bobzhang/crescent/httputil @httputil for set_missing_header_case_insensitive (used by the String/ToJson impls to set Content-Type)
  • moonbitlang/core/buffer, moonbitlang/core/json, moonbitlang/core/encoding/utf8 for impl bodies

Call sites to update

Any file that writes &Responder in a type annotation gets &@responder.Responder. Main hotspots: dispatch.mbt, typed_handler.mbt, middleware.mbt (the Middleware type alias), serve_async.mbt, handler.mbt. All are type annotations — no runtime changes. Method dispatch ("hi".to_responder(), event.res.to_responder()) works across package boundaries without qualification.

Breaking API changes

// Before
pub type Middleware = async (Event, MiddlewareNext) -> &Responder noraise

// After
pub type Middleware = async (Event, MiddlewareNext) -> &@responder.Responder noraise

Downstream callers that explicitly annotate &Responder must import @responder. Users who rely on inference (the common case) are unaffected.

Verification

  • moon check clean
  • moon test 504/504 passing
  • moon info && moon fmt — root pkg.generated.mbti loses the Responder trait + impls; responder/pkg.generated.mbti gains them. All HttpResponse::ok/... and redirect helpers stay in root's mbti.

Phase 3 — websocket/ extraction (revised after codex review)

Goal

Extract websocket.mbt (types: WebSocketPeer, WebSocketEvent, WebSocketAggregatedMessage, WebSocketHandler type alias) and websocket_async.mbt (native runtime: ws_send, ws_subscribe, ws_publish, etc.) into a new bobzhang/crescent/websocket sub-package.

Constraint discovered by codex review

  • App::ws on current main already registers handlers into private ws_static_routes / ws_dynamic_routes radix storage on App (index.mbt).
  • serve_async.mbt dispatches WebSocket routes by reading those fields directly.
  • So no router API change is needed — the original "Option A/B prerequisite" in the draft was stale. App::ws stays at root (method coherence); only the types and runtime move.

What moves

  • websocket.mbt → websocket/websocket.mbt (types + handler alias + any free functions)
  • websocket_async.mbt → websocket/websocket_async.mbt (~496 LOC native runtime)
  • websocket_async_native_wbtest.mbt → websocket/websocket_async_native_wbtest.mbt — kept as wbtest. Codex confirmed it inspects native_ws_hubs, NativeWebSocketConnection, and private runtime IDs; a blackbox rewrite would lose coverage.

What stays at root

  • App::ws method (public, pinned by coherence)
  • ws_static_routes / ws_dynamic_routes fields on App
  • serve_async.mbt's WebSocket upgrade path

New dependency direction

Root's index.mbt (App struct) uses WebSocketHandler as a field type. After the extraction, that field type is @websocket.WebSocketHandler. Root's moon.pkg needs "bobzhang/crescent/websocket" @websocket in the main import block, not just for "test", because it's load-bearing for the App struct definition. websocket/ itself must NOT import root — otherwise cycle.

Codex warning: this works only if websocket/ is root-independent. If any function in websocket_async.mbt references App or other root types, it has to either (a) move its use-site to root or (b) take the root type as a parameter rather than importing it. Review websocket_async.mbt carefully during implementation.

Breaking API changes

// Type alias in App field
priv ws_static_routes : Map[String, WebSocketHandler]
// becomes
priv ws_static_routes : Map[String, @websocket.WebSocketHandler]

For users: WebSocketPeer, WebSocketEvent, etc. all move to @websocket.*. Call sites in examples/websocket_echo.mbt or similar need updating.

Verification

  • moon check — catches any root-dependency leak in the websocket package
  • moon test — WebSocket integration tests stay green
  • moon info && moon fmt — root mbti loses WebSocket types, websocket mbti gains them

Cross-cutting concerns

Commit/PR flow per phase

Each phase is one branch, one commit, one PR. No phase depends on a later phase's work.

hongbo/phase-1-middleware       → bobzhang/crescent/middleware
hongbo/phase-2-responder        → bobzhang/crescent/responder
hongbo/phase-3-static-merge     → static_file/ consolidation
hongbo/phase-4-websocket        → bobzhang/crescent/websocket

Match the Round-1/Round-2 commit style: Extract <thing> into bobzhang/crescent/<pkg> sub-package.

Regenerating pkg.generated.mbti

After each phase: moon info --format mbti (or moon fmt per project convention — check AGENTS.md). Include the regenerated mbti file in the same commit.

README.mbt.md doctest updates

Every phase must update the relevant mbt check / moonbit check doctest blocks in root README.mbt.md. Phase 1's plan lists 4 locations; later phases will have similar fan-out. The for "test" import block of root's moon.pkg accumulates a new entry per phase.

Examples / benchmarks

examples/ and benchmarks/ are separate modules per moon.mod. Each phase updates the relevant example's moon.pkg imports and source call sites. Track this per phase — don't batch.

Review gating

Every phase ends with codex reviewing the implementation diff before the PR opens. Workflow per phase:

  1. I update this roadmap + the phase's detail notes.
  2. codex reviews the detail notes (text-only review, before any code moves).
  3. I implement the phase on a branch.
  4. moon check && moon test green locally.
  5. codex reviews the diff (git diff origin/main..HEAD).
  6. I fix any [P1]/[P2] feedback.
  7. PR opens; auto-merge once CI is clean.
  8. Next phase starts.

Phases do not overlap — Phase 2 starts only after Phase 1 merges. This keeps every phase rebaseable on a clean main and lets codex review a focused diff each time.

Questions for codex on this roadmap

  1. Phase ordering. Is middleware → responder → static-merge → websocket the right order, or should static-merge come first because it's pure cleanup? My argument for middleware first: .architecture-plan.md already has a codex-reviewed plan and unblocks the most code (~400 LOC).

  2. Scope of Phase 2. Is bundling responder + response_helpers + redirect into one package the right call, or should it be two packages (responder/ for the trait + factories, redirect/ separately)? The Explore survey says they're all "how to build an HttpResponse" and should stay together. I agree. Sanity check.

  3. Phase 4 prerequisite (router API). Is Option A (pub(r) field) or Option B (accessor method) more idiomatic in MoonBit? Is there a third option I'm missing?

  4. Anything I should NOT extract. The "deferred" section at the top lists TestClient, typed handlers, server runtime, and fetch as things I'm explicitly leaving at root. Any of those actually worth extracting despite the complications I described? If yes, which and why?

  5. Risk of regressions. The biggest risk I see is the README.mbt.md mbt check blocks going stale after each phase. Is there a way to dry-run just those blocks without running the full moon test?

Keep the review focused and actionable — I want to implement, not re-plan. Tag each finding [P1 blocker], [P2 should fix], [P3 nit].