Improve saga dispatch concurrency and cleanup - #33
Open
misak113 wants to merge 2 commits into
Open
Conversation
Index combined sagas by handled action type, run matching leaf sagas independently, and track action completion without mutating frozen actions. Clean up finished iterators and observable subscriptions while preserving legacy action promise compatibility. Ported from signageOS Box MR 3682 commit a304d1d005f846f00261b402aff0a912653ba128. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Port follow-up behavior from Box MR 3682, including updater routing, reducer delivery, fan-out settlement, completion callbacks, error normalization, and lifecycle cleanup.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors Aperol’s saga execution model to route updater construction by declared actionTypes, run matching leaf sagas concurrently, improve error propagation/reporting, and make observable subscription cleanup deterministic. It also introduces a WeakMap-backed action completion promise API to support frozen/reused actions, and updates documentation + tests accordingly.
Changes:
- Add
actionTypesdeclarations to sagas and introduceANY_ACTION/ saga-group matching to route only relevant updaters while reducers still receive every action. - Execute matching leaf sagas concurrently, wait for all siblings to settle, and normalize/report updater rejections (including nested-action failure ownership).
- Replace direct
action.__promisemutation with a WeakMap-backedActionPromisehelper (while keeping legacy__promisecompatibility), plus improved observable subscription lifecycle handling.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/sumModelMock.ts | Adds actionTypes declarations to test sagas for updater routing. |
| tests/unit/createModelSaga.spec.ts | Expands unit coverage for concurrency, action promise behavior, rejection normalization, callbacks, and subscription cleanup. |
| tests/unit/combineSagas.spec.ts | Adds tests for required actionTypes, reducer-vs-updater routing behavior, wildcard routing, and deduped routes. |
| tests/unit/ActionPromise.spec.ts | New unit tests for WeakMap-backed action promise storage and legacy compatibility. |
| tests/integration/Profiler/profiler.spec.ts | Updates profiler integration expectations to include sagaKey and nested saga keys. |
| tests/integration/index.spec.ts | Verifies new public exports (ANY_ACTION, getActionPromise) are available from the package entrypoint. |
| src/SagaGroup.ts | Introduces saga-group matching (MATCH_SAGAS), leaf saga identification, and ANY_ACTION. |
| src/Profiler/profiler.ts | Adds saga-key-aware profiler warnings and reduces profiling overhead when disabled. |
| src/ISaga.ts | Extends saga contract to require actionTypes for updater routing. |
| src/index.ts | Exports ANY_ACTION and getActionPromise publicly. |
| src/createModelSaga.ts | Implements concurrent leaf saga execution, normalized error handling, completion callbacks, and deterministic observable subscription cleanup. |
| src/combineSagas.ts | Builds actionType->saga routing index, supports nested groups, preserves reducer identity, and keeps a flattened updater fallback. |
| src/ActionPromise.ts | Adds WeakMap-backed promise storage with best-effort legacy __promise attachment. |
| README.md | Documents required actionTypes, wildcard usage, and onUpdateComplete contract. |
| CHANGELOG.md | Records new concurrency/routing behavior and related fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+285
to
+290
| const matched: IMatchedSaga[] = []; | ||
| if (isSagaGroup<TModel>(saga)) { | ||
| saga[MATCH_SAGAS](model, action, '', matched); | ||
| } else if (saga.actionTypes.indexOf(action.type) !== -1 || saga.actionTypes.indexOf(ANY_ACTION) !== -1) { | ||
| matched.push({ key: '', iterator: saga.updater(model, action) }); | ||
| } |
| return action.type === ObservableSubscribed; | ||
| } | ||
|
|
||
| describe('Application.craeteModelSaga', function () { |
Owner
Author
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
actionTypeswhile delivering every action to every reducer, with duplicate route declarations deduplicated.Errorrejection normalization, safe callback handling, and deterministic observable/subscription cleanup.Compatibility
actionTypes; useANY_ACTIONfor wildcard updater routing.onUpdateCompletecontract.Validation
npm run lintnpm test— 33 passingnpm run buildSource
a304d1d60d6b52