feat(wiring): library composition with cross-package types#333
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #333 +/- ##
==========================================
+ Coverage 97.57% 97.74% +0.17%
==========================================
Files 24 24
Lines 1276 1373 +97
Branches 251 288 +37
==========================================
+ Hits 1245 1342 +97
Misses 30 30
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
hfesel-rvoh
left a comment
There was a problem hiding this comment.
Comments with the help of Claude! Otherwise it looks good. The implies with no depends seems like the biggest risk to me
| ): void | never { | ||
| // rollups are nameless membership carriers — exclude them here. The flatten pass | ||
| // expands them to their (named) members, which are validated on the post-flatten list. | ||
| const definitions = libraries.filter(entry => !isRollup(entry)) as LibraryDefinition< |
There was a problem hiding this comment.
assertLibraryNames now filters rollups out before counting, but it's also called from CreateApplication (assertLibraryNames(libraries), ~line 413) on the un-flattened input. So a RollupLibraries([...]) containing a same-name/different-object collision is no longer caught at definition time — only at bootstrap after flattenLibraries expands it. That contradicts the @throws DUPLICATE_LIBRARY / "fail loudly at definition time" promise in the CreateApplication TSDoc (line 384) and assertLibraryNames's own doc. Either expand rollups in the definition-time check, or update those doc comments to note rollup contents are validated at bootstrap. (Bootstrap does catch it — the rejects same-name/different-object delivered via a rollup test confirms — so this is doc accuracy, not a hole.)
| * Distinct from `depends`: `depends` is ordering + validation; `implies` is membership. | ||
| * Rollups are accepted here and flattened recursively. | ||
| */ | ||
| implies?: readonly RollupMember[]; |
There was a problem hiding this comment.
Worth making the ordering hazard explicit here since this is the field's doc: implies contributes membership with no ordering edge, so a consumer that sets only implies: [X] (not depends: [X]) and reads params.X in the factory body — rather than inside a lifecycle hook — can get undefined if X wires after the implier. The README and demo cover it (the demo sets both), but a one-line @remarks pointing readers to pair implies with depends when they touch the member at wire time would prevent the obvious misuse.
…roof is enforceable
…ms; correct LibraryGroup docstrings to match runtime
…c as a local analogue
…-command remediation
…; flag the implies wire-time ordering hazard
…ING_DEPENDENCY required-dep throw
Changes
LibraryGroup({ name?, members, registry? })to compose libraries as a single membership unit (replaces the unmergedRollupLibraries).registry: "<name>"generates thepriorityInitregistry-service wiring (the plugin-registry pattern).dependsnow resolves transitive membership automatically (closure-as-membership, identity-deduped); every auto-pulled library is narrated in the boot log.impliesanddependscarry member types across package boundaries with no manual re-export; directly-listed types take priority over hoisted ones.DUPLICATE_LIBRARYis a fixed three-case rule with a fact-only message (no remediation prose).yarn type-checkas a CI gate and ayarn test:cross-packageproof for the cross-package type channel.Companion docs PR: Digital-Alchemy-TS/documentation#68