refactor: internal patterns (errors helper, DRY, dispatch tables) — v1 prep 1/3#1
Merged
Merged
Conversation
Code-level reference covering layering rules, file/directory naming, ESM resolution conventions, core code patterns (factory + frozen loader, define-time vs load-time validation, CLI subcommand triplet, generator purity, error throwing, preset pattern, build-tool plugin pattern), testing strategy, the new-feature checklist, and anti-patterns. Complements AGENTS.md (what the library does) and CONTRIBUTING.md (dev loop) — focuses on the code patterns a new contributor must follow to keep changes consistent.
- Add raise(code, msg, opts): never to errors.ts, replacing 18
throw new NodeSettingsError(...) sites across define-settings,
client-env, and validate-options.
- New utils/zod-issues.ts centralises ZodError → text and
ZodError → {path, message}[] conversion. Removes three
near-identical inline formatters (define-settings, client-env,
cli/validate).
- Move assertSettingsLoaderShape into validate-options.ts and
share it between defineSettings's eager extends check and the
full validateDefineSettingsOptions pass — was duplicated.
- Collapse the parallel ZodEnum / ZodNativeEnum branches in
validate-options into a single enumValuesOf() helper.
Internal-only. No public API or error-message changes.
cli/generate.ts: The 90-line switch statement on the <target> positional becomes a HANDLERS registry — each entry declares its aliases and a self-contained run(loader, args) function. Adding a new generator now means adding one entry to the array; the dispatch is a single Map lookup. Each handler returns an outcome tagged with kind: 'output' | 'done' | 'exit' so the control flow is explicit. optionalString() consolidates the repeated 'flag is set → put in opts' spread idiom. introspect.ts: Replace the if/else if chain over inner ZodType names with PRIMITIVE_TYPE_RESOLVERS, a record keyed by typeName. Extending support for new primitive types now only requires adding one entry to the map.
cli/check.ts, cli/inspect.ts, and cli/preflight.ts all duplicated the
same "discover workspace → iterate packages → print header + banners"
shell. New cli/workspace-runner.ts centralises:
- setupWorkspaceRun(args) workspace root discovery + 'no packages'
error (consistent across subcommands)
- printWorkspaceHeader() 'workspace root: …\npackages: N\n' header
- printPackageBanner() '=== name (rel/path) ===' banner
- printPackageError() ' [node-settings] <error>'
Each subcommand keeps its bespoke per-package result type and
aggregation logic — the helpers only own the bits that were
character-for-character identical. Dropped one inconsistent
'scanned: packages/*' hint from check (was check-only and stale once
pnpm/npm workspace config is present).
setupWorkspaceRun() emits a 'no packages found' error in both text and --format=json modes, but no existing test exercised it. Adding the two missing tests recovers branch coverage to 80.34% (above the 80% floor) after the workspace-runner refactor introduced the helper. (Split from the original c9882b4; the verify-dist sync for the loadViteEnv removal lands separately alongside that removal.)
Drop comments that paraphrase the line below them. Identifiers and
control flow already communicate the 'what'; the remaining comments
should justify a choice or surface a non-obvious constraint.
Touched files:
- validate-options.ts step-number banners ('1. envSchema must…',
'2. envKey must…', …) removed. The throw codes
right under each block are the same labels.
Kept the WHY tails on the enum-subset and
eager-extends checks.
- presets.ts 'Platform presets' ASCII divider — type
names already structure the file.
- check-per-env.ts 'Leaf — test the leaf path' — restates the
type-narrowing the branch already proved.
- introspect.ts 'Walk wrapper layers until we hit the
underlying primitive' — what the loop does.
- dotenv-cascade.ts tightened step-numbered banners; kept the
non-obvious WHY (load-order rationale,
skipped-entry semantics, appEnvKey fallback).
- env-example.ts 'Trim trailing blank line' — the .pop()
loop says this directly.
- workspace.ts empty '// skip' catch comment replaced with
a block comment that names the actual reason.
- define-settings.ts removed 'Now validate the merged shape…'
— function name already says so.
Net change: -33 comment lines. No behavior change.
Three small consistency fixes across NodeSettingsError messages:
- Add trailing period to the two 'Known keys: …' hints in
validate-options.ts so they match every other hint in the package.
- Add trailing period to 'Known branches: …' in PER_ENV_BRANCH_MISSING
for the same reason.
- 'yaml input not found' / 'failed to read yaml input' → 'YAML' so
the case matches 'failed to parse YAML' and 'invalid YAML' a few
lines away in diff-k8s.ts.
Single-sentence messages always end with a period; messages that
end with an embedded cause (':' + err.message) or a multi-line list
do not — that convention is now uniform.
This was referenced May 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First of three stacked PRs preparing for v1.0.0. Internal-only refactoring — no public API changes; the published surface is unchanged.
Lays the foundations the next two PRs build on:
docs/ARCHITECTURE.mddocuments the layering rules, file/dir naming, ESM resolution, and the seven core code patterns the codebase follows.raise(code, msg, opts)helper inerrors.tsreplaces 18 directthrow new NodeSettingsError(...)sites.utils/zod-issues.tsconsolidates three duplicatedZodError → path/messageformatters into one source of truth.cli/generate.tsrewritten around aHANDLERSdispatch table instead of a 90-line switch.introspect.tsprimitive-type detection moved from an if/else chain to aPRIMITIVE_TYPE_RESOLVERSlookup.cli/check.ts,cli/inspect.ts,cli/preflight.tsshare a newcli/workspace-runner.tsfor workspace setup, header rendering, and per-package banners (~80 lines of duplicated boilerplate removed).Commits
Stacked PRs
This is 1/3. The next two stack on top of this branch:
feat/error-catalog-v1— new error codes,loadViteEnvremoval (breaking),ERROR_CATALOG+reportError().docs/test-taxonomy-and-final—test:unit/integ/e2e/contractscripts +docs/TESTING.md+ final README/AGENTS/ARCHITECTURE refresh.Merge in order so each diff is reviewable in isolation.
Test plan
pnpm typecheck— cleanpnpm test— 294 passingpnpm verify— 8-layer chain (verify:errorsarrives in 2/3)docs/ARCHITECTURE.md— does the layering description match what you'd expect a new contributor to need?