fix: resolve params once per target, not per edge, in the transitive dependency walk - #22
Open
neilverc wants to merge 1 commit into
Open
fix: resolve params once per target, not per edge, in the transitive dependency walk#22neilverc wants to merge 1 commit into
neilverc wants to merge 1 commit into
Conversation
…transitive dependency walk PLAT-8324 decided the policy for a target reached by multiple edges in veil's transitive dependency walk (PLAT-8321): the render root's own direct declaration wins over any transitive path, and two indirect paths disagreeing on params with no direct declaration to arbitrate is a hard render-time error. PR #20/#21 shipped the BFS walk and per-hop-consumer mechanics but never implemented this policy — applyDependencies fired a target's dependent hooks once per incoming edge, unconditionally, with that edge's own (unmerged) params. applyDependencies now runs in two passes: first it walks the graph collecting every (declarer, target) edge without applying hooks, then for each distinct target it resolves a single params object via resolveDependencyParams (root's direct edges win when present; otherwise every indirect edge must agree, or it's a hard error) and applies that target's dependent hooks exactly once. Replaces TestDiamondDependencyFiresTargetHookOncePerIncomingEdge (which asserted the old double-fire behavior) with three tests covering the new policy: agreeing indirect params fire once, conflicting indirect params hard-error, and a root-direct declaration overrides a conflicting transitive one. Updates SPEC.md's render-time execution section accordingly.
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.
Problem
PLAT-8321/#20 added the full transitive dependency walk; #21 fixed
ctx.consumerto always be the render root. Neither implemented PLAT-8324's decided policy for a target reached via multiple edges:applyDependenciesfired a target's dependent hooks once per incoming edge, unconditionally, with that edge's own unmerged params — so a target declared both directly by the render root and transitively through another dependency fired twice, with two different params, instead of the root's declaration winning.Resolution
Per PLAT-8324: visit each
(kind,name)target exactly once for hook application (not once per edge).applyDependenciesnow runs two passes:(declarer, target)edge, without applying hooks.resolveDependencyParamspicks a singleparamsobject:Tests
Replaces
TestDiamondDependencyFiresTargetHookOncePerIncomingEdge(asserted the old double-fire behavior) with:TestDiamondDependencyWithAgreeingIndirectParamsAppliesHookOnce— two indirect paths agreeing on params fire once.TestConflictingIndirectDependencyParamsIsHardError— two indirect paths disagreeing, no direct declaration → hard error.TestRootDirectDependencyOverridesConflictingTransitiveParams— root declares a target directly and reaches it transitively with different params → root's params win, hook fires once, indirect params never take effect.Existing
TestMultiHopDependencyAppliesTransitiveDependentHooks" andTestDependencyCycleAppliesEachEdgeOnceWithoutInfiniteLoop` are unaffected (each target there has exactly one edge).SPEC.md's render-time execution section documents the new params resolution policy.