Repository navigation
docs: document how a task result is derived - #46
Conversation
A task function may return a plain value, a Promise or an Observable, but the way an Observable is consumed was not documented anywhere. The first emitted value becomes the task's result and the subscription is closed right after it, so later emissions never occur and the source is cancelled. The cancellation is the surprising part: a request that reports progress resolves with its first progress event and is then aborted. Document this on the Task type, on register() and in the README, together with the last() and toArray() alternatives and the caveat that last() never emits for a source that does not complete. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_018q1BpbY6mjVrbgaPBQDA5Z
🦋 Changeset detectedLatest commit: d6f7ffe The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR documents task result resolution for values, Promises, and Observables. It specifies first-emission Observable behavior, cancellation, alternate collection operators, non-completing sources, and ChangesTask result documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 165-199: Update the Observable examples in the README to include
or explicitly share imports for Resolver, of, lastValueFrom, and EMPTY; add the
missing rxjs symbols and import EMPTY from `@robinw151/resolver`, while preserving
the existing example behavior.
- Around line 178-180: Update the README progress example to observe the full
HttpClient event stream and filter it to progress events before resolving or
claiming cancellation behavior. Ensure the example reflects that the first
emitted event may be HttpEventType.Sent and that reportProgress alone does not
provide progress events.
In `@src/resolver.interface.ts`:
- Around line 30-60: Align the Observable contract documentation in Task
(src/resolver.interface.ts:30-60), the resolver documentation
(src/resolver.ts:146-150), README.md:163-192, and the release note
(.changeset/document-task-results.md:5-8): state that later emissions are not
observed after unsubscription and upstream work stops only when the source
honors teardown. Add the completion risk of using toArray() to all three
task-result docs and the release note, and use “afterward” in the release note.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ec9751c-3046-4dc8-9821-882a5f642455
📒 Files selected for processing (4)
.changeset/document-task-results.mdREADME.mdsrc/resolver.interface.tssrc/resolver.ts
Address review feedback: - Fix the HttpClient progress example. With reportProgress alone the request is still observed as a body, so it emits only the response and the example contradicted the point it was making. Observed as events it emits HttpEventType.Sent first, which is the behaviour worth warning about. - Say that later emissions are never observed and that sources honoring unsubscription are cancelled, instead of claiming the source is always cancelled. Unsubscribing only stops upstream work when the source implements teardown. - Extend the completion caveat to toArray(), which carries the same risk as last(). - Complete the imports in the README examples. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_018q1BpbY6mjVrbgaPBQDA5Z
|
2 participants
Problem
A task function may return a plain value, a
Promiseor anObservable, but the way anObservableis consumed was not documented anywhere. TheTasktype had no JSDoc at all.The first emitted value becomes the task's result and the subscription is closed right after it. The value selection is guessable; the cancellation is not:
Why the behaviour is documented rather than changed
Aligning with
forkJoin's last-value convention would reintroduce the hang fixed in #43.interval(1000)currently resolves with its first value; underlast()semantics it would never emit and never complete, leaving its promise pending and stalling the whole graph. Any never-completing source (websocket, subject-backed stream, long poll) would become a graph-wide hang.Both alternatives are already expressible by the caller, so the default blocks no use case — it just picks the one that cannot hang:
of(1, 2, 3)1of(1, 2, 3).pipe(last())3of(1, 2, 3).pipe(toArray())[1, 2, 3]interval(1000)0, no hangA per-task
emission: 'first' \| 'last'option was considered and rejected: it is redundant withpipe(last()), and its'last'branch would exist only to let a caller deadlock the resolver.Changes
Documentation only, no behaviour change.
Tasktype (src/resolver.interface.ts) — the "exactly one result" contract on the interface, and onfnthe first-emission rule, the cancellation,EmptyTaskErrorfor empty completion, and thelast()/toArray()alternatives. This is the version that reaches editors through the emitted.d.ts.register()JSDoc — a condensed version on thetaskparameter, where the task is actually being written.EmptyTaskErroris exported and carries thetaskId.Each spot leads with the cancellation rather than the value selection, and flags that
last()never emits for a source that does not complete, which would leave the resolution pending indefinitely.Verification
Every README snippet was extracted into a temporary type-spec and compiled against the public
src/indexentry point. All compile, including theisErrornarrowing before theinstanceof EmptyTaskErrorcheck — the naive form without narrowing would not have typechecked. The scratch file was removed afterwards.vitest run --project node— 38/38tsc --noEmit --project tests/tsconfig.jsoneslint,prettier --checkThe browser project could not be run in my environment (the preinstalled Chromium build does not match the pinned
playwrightversion).A
patchchangeset is included, since the JSDoc ships to consumers in the type declarations.Generated by Claude Code
Summary by CodeRabbit
last()andtoArray().EmptyTaskErrorhandling and task identifiers.