refactor: remove dead _failedTasks field and unused async-pageable.ts#284
Open
YunchuWang wants to merge 1 commit into
Open
refactor: remove dead _failedTasks field and unused async-pageable.ts#284YunchuWang wants to merge 1 commit into
YunchuWang wants to merge 1 commit into
Conversation
The `_failedTasks` field on `CompositeTask` was declared and initialized to 0 but never incremented or read anywhere in the repo (confirmed by grep). Neither `WhenAllTask.onChildCompleted()` nor `WhenAnyTask.onChildCompleted()` touched it. It misleadingly implied failed children were tracked separately from `_completedTasks`, when in fact `WhenAllTask` counts every settled child -- successful or failed -- in the single `_completedTasks` counter. Removed the field and updated the now-stale comment in `WhenAllTask` that referenced it. `src/utils/async-pageable.ts` was an unimported `AsyncPageable` class superseded by the real pagination implementation in `src/orchestration/page.ts` (exported from index.ts and used by client.ts and the export-history client). No file referenced it by path or symbol, and it was not re-exported from any barrel, so deleting it is safe -- the core build stays green. Added unit tests documenting the real behavior: `WhenAllTask.completedTasks` counts both successful and failed child completions, and `WhenAnyTask` settles on the first completed child regardless of success/failure without tracking failures separately. No behavior change. Fixes #217 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Dead-code cleanup in the core DurableTask JS SDK to reduce confusion and better document actual composite task semantics, without changing runtime behavior.
Changes:
- Removed the unused
_failedTasksfield fromCompositeTaskand updated the related stale comment inWhenAllTask. - Deleted the unused
src/utils/async-pageable.tsimplementation (pagination is provided bysrc/orchestration/page.ts). - Added unit tests to document that
WhenAllTask.completedTaskscounts both successful and failed child completions, and thatWhenAnyTasksettles on the first completed (even failed) child without later override.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/durabletask-js/src/task/composite-task.ts | Removes dead _failedTasks field from composite task state. |
| packages/durabletask-js/src/task/when-all-task.ts | Updates constructor comment to match current composite task behavior. |
| packages/durabletask-js/src/utils/async-pageable.ts | Deletes unused pageable implementation to avoid duplicate/confusing pagination code. |
| packages/durabletask-js/test/when-all-task.spec.ts | Adds regression test documenting completedTasks increments for both success and failure. |
| packages/durabletask-js/test/when-any-task.spec.ts | Adds regression test documenting first-settled-child behavior even when the first completion is a failure. |
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
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
Dead-code cleanup in the core SDK. No behavior change — build, lint, and the full core test suite (1051 tests) all pass.
What & why
1. Removed the
_failedTasksfield fromCompositeTask_failedTaskswas declared and initialized to0in the constructor but never incremented or read anywhere in the repository. I confirmed this with a repo-wide search — the only occurrences were the declaration, the constructor init, and a comment:Neither
WhenAllTask.onChildCompleted()norWhenAnyTask.onChildCompleted()ever touched it. The field was misleading: it implied failed children were tracked separately, when in realityWhenAllTaskcounts every settled child — successful and failed — in the single_completedTaskscounter (it increments before the fail-fast check). A separate failure counter was never needed.2. Fixed the stale comment in
WhenAllTaskThe constructor comment warned against re-initializing
_completedTasksor_failedTasks. With_failedTasksgone, the comment now references only_completedTasksso it stays accurate.3. Deleted the dead
src/utils/async-pageable.ts(91 lines)This file contained an
AsyncPageableclass /Pageinterface /PageFetchertype that was never imported by any file. The real pagination implementation lives insrc/orchestration/page.ts, which is exported fromindex.tsand consumed byclient.tsand the export-history client. Verified before deleting:grep "async-pageable"→ only the file itself).AsyncPageable/createAsyncPageableimport resolves toorchestration/page.ts(note:async-pageable.tsdidn't even exportcreateAsyncPageable).utils/index.ts).The core build stays green after deletion, confirming nothing depended on it.
4. Added tests documenting the real behavior
To capture the intent that
_failedTasksmisleadingly implied was separate:when-all-task.spec.ts— verifiescompletedTaskscounts both a successful child and a failed child (single counter, no separate failure count).when-any-task.spec.ts— verifiesWhenAnyTasksettles on the first completed child regardless of success/failure and does not track failures separately (a later successful child does not override an already-settled failed child).Verification
npm run build:core— clean compile (confirms deletingasync-pageable.tsbroke nothing).npx eslinton all changed files — exit 0.npx jest --runInBand(full core suite) — 59 suites, 1051 tests, all pass (includes the 2 new tests).Fixes #217