Skip to content

feat: step-level caching, async hooks, onRunComplete output#27

Draft
brianjenkins94 wants to merge 4 commits into
danfry1:mainfrom
brianjenkins94:feat/caching-async-hooks-run-complete
Draft

feat: step-level caching, async hooks, onRunComplete output#27
brianjenkins94 wants to merge 4 commits into
danfry1:mainfrom
brianjenkins94:feat/caching-async-hooks-run-complete

Conversation

@brianjenkins94
Copy link
Copy Markdown

Addresses interrelated issues #21, #23, #24. More detail to come.

…c createEngine

_createEngine becomes the internal implementation. The public createEngine
supports both single-workflow (enqueue infers name) and multi-workflow
(enqueue requires name) forms with full type inference preserved.
@brianjenkins94 brianjenkins94 force-pushed the feat/caching-async-hooks-run-complete branch from f76cf46 to 5f654af Compare May 3, 2026 16:36
@danfry1
Copy link
Copy Markdown
Owner

danfry1 commented May 13, 2026

Hey Brian — really nice draft. After shipping #31 I went back through this and I think there's a strong case for splitting out the async hooks change (#23) into its own small PR rather than landing all three together. Wanted to lay out exactly what we'd want in scope and offer to collaborate on it.

Why split

  • [Feature Request] Support async hooks #23 is surgical and low-risk — every hook call site gets awaited and the silent catch {} becomes a logged error. No new types, no new public surface, no storage migration. Easy review, easy revert if something surprises us.
  • [Feature Request] Some way to conditionally run a step, but still supply the last cached result to subsequent steps #21 (caching) needs a design pass first — adding getCachedStepResult to StorageAdapter is a minor-version-bumping change that all storage authors need to follow. There are also a few open API questions worth nailing down in the issue thread before code (e.g. is cacheKey required when cache is set, or do we just silently no-op? what's the eviction story?). I'd rather decide those before locking in the shape.
  • What if the step function could be an AsyncGenerator? #24 (runner) — I had reservations about this one in my read of the issues; mostly it's that the proposed engine.results() shape is single-consumer and engine-wide, and I'd want to see another use case before baking that into the library. Happy to keep talking on the issue.

Splitting also means we get #23 in front of users faster and you get a clean merge while we continue the conversation on the other two.

What we'd want in the #23 PR

In scope:

  • All five hooks become Promise<void> | void in EngineHooks: onRunStart, onStepStart, onStepComplete, onRunComplete, onRunFailed. Plus onError.
  • Engine awaits them inline at every call site (this is the actual behavioral change — back-pressure matters for users who do storage writes / metrics flushes inside hooks).
  • Hook errors are logged via console.error('[reflow-ts] <hookName> threw:', err) rather than swallowed by empty catch blocks. Hook errors must not fail the run — that part of today's behavior we want to keep.
  • Your existing tests in src/core/__tests__/async-hooks.test.ts are exactly right — sync throw, async reject, hook-throws-don't-fail-the-run. Carry those over as-is.

Out of scope (defer to their own PRs):

One small heads-up: #31 merged a parallel-execution path that adds two new onStepStart / onStepComplete call sites inside executeParallelGroup. They'll need the same await + console.error treatment when you rebase. I can do that part if it helps.

Collaboration

Happy to:

  • Open the extraction PR myself if you'd rather not redo it (you'd be co-author on the commits, all the design + test work is yours)
  • Or review yours, fast — should be a same-day turnaround on something this size

Either way, your call. Whatever's least friction for you.

Thanks again for putting all this together — the test coverage on the hook failure modes is really thoughtful.

@brianjenkins94
Copy link
Copy Markdown
Author

brianjenkins94 commented May 13, 2026

No problem, I just needed all these things so I thought I'd take a stab at it.

It'll probably be a couple weeks before I have time to work on this again, so don't let me hold you up if you want to build this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants