Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(store): cancel uncompleted should unsubscribe before next #1763

Merged

Conversation

markwhitfeld
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Currently there is a slightly unexpected order of events when one in-process action is cancelled in favor of another.
The handler for the second action is executed before the observable for the first actions is unsubscribed.
This can lead to subtle issues, especially when implementing an optimistic update strategy.

Issue Number: N/A

What is the new behavior?

After this PR, the first action's observable will be unsubscribed (and cancelled) before the second action starts executing its handlers.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@markwhitfeld markwhitfeld added this to the v3.next-minor milestone Jun 17, 2021
@markwhitfeld
Copy link
Member Author

markwhitfeld commented Jun 17, 2021

@splincode @arturovt Ready to go!
Review please @arturovt 😉

@arturovt
Copy link
Member

Let’s go!

@arturovt arturovt merged commit bbf2968 into master Jun 17, 2021
@arturovt arturovt deleted the fix/cancel-uncompleted-should-unsubscribe-before-next branch June 17, 2021 14:29
@markwhitfeld markwhitfeld modified the milestones: v3.next-minor, v3.7.4 Jun 10, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 17, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@ngxs/form-plugin](https://github.com/ngxs/store) | dependencies | patch | [`3.7.3` -> `3.7.4`](https://renovatebot.com/diffs/npm/@ngxs%2fform-plugin/3.7.3/3.7.4) |
| [@ngxs/storage-plugin](https://github.com/ngxs/store) | dependencies | patch | [`3.7.3` -> `3.7.4`](https://renovatebot.com/diffs/npm/@ngxs%2fstorage-plugin/3.7.3/3.7.4) |
| [@ngxs/store](https://github.com/ngxs/store) | dependencies | patch | [`3.7.3` -> `3.7.4`](https://renovatebot.com/diffs/npm/@ngxs%2fstore/3.7.3/3.7.4) |

---

### Release Notes

<details>
<summary>ngxs/store</summary>

### [`v3.7.4`](https://github.com/ngxs/store/blob/HEAD/CHANGELOG.md#&#8203;374-2022-06-09)

[Compare Source](ngxs/store@v3.7.3...v3.7.4)

-   Build: include support for Angular 14 [#&#8203;1850](ngxs/store#1850)
-   Fix: Do not re-use the global `Store` instance between different apps [#&#8203;1740](ngxs/store#1740) and [#&#8203;1804](ngxs/store#1804)
-   Fix: Handle mixed async scenarios for action handlers [#&#8203;1762](ngxs/store#1762)
-   Fix: An action with cancelUncompleted enabled should unsubscribe before the next action handler is called [#&#8203;1763](ngxs/store#1763)
-   Fix: Do not run `Promise.then` within synchronous tests when decorating factory [#&#8203;1753](ngxs/store#1753)
-   Fix: Provide `NoopNgxsExecutionStrategy` explicitly when the zone is nooped [#&#8203;1819](ngxs/store#1819)
-   Fix: Complete the state stream once the root view is removed [#&#8203;1830](ngxs/store#1830)
-   Fix: Be more explicit when checking if Angular is in test mode [#&#8203;1831](ngxs/store#1831), [#&#8203;1832](ngxs/store#1832)
-   Fix: Devtools Plugin - Do not connect to devtools when the plugin is disabled [#&#8203;1761](ngxs/store#1761)
-   Fix: Router Plugin - Cleanup subscriptions when the root view is destroyed [#&#8203;1754](ngxs/store#1754)
-   Fix: WebSocket Plugin - Cleanup subscriptions and close the connection when the root view is destroyed [#&#8203;1755](ngxs/store#1755)
-   Fix: Storage Plugin - Only restore state if key matches `addedStates` [#&#8203;1746](ngxs/store#1746)
-   Fix: Forms Plugin - Do not destructure primitive types [#&#8203;1845](ngxs/store#1845)
-   Performance: Tree-shake errors and warnings [#&#8203;1732](ngxs/store#1732)
-   Performance: Tree-shake `ConfigValidator`, `HostEnvironment` and `isAngularInTestMode` [#&#8203;1741](ngxs/store#1741)
-   Performance: Tree-shake `SelectFactory` [#&#8203;1744](ngxs/store#1744)
-   Performance: Tree-shake `deepFreeze` [#&#8203;1819](ngxs/store#1819)
-   Performance: Run change detection once for all selectors when asynchronous action has been completed [#&#8203;1828](ngxs/store#1828)
-   Performance: Router Plugin - Tree-shake `isAngularInTestMode()` [#&#8203;1738](ngxs/store#1738)
-   Performance: Tree-shake `isAngularInTestMode()` [#&#8203;1739](ngxs/store#1739)
-   Performance: Storage Plugin - Tree-shake `console.*` calls and expand error messages [#&#8203;1727](ngxs/store#1727)
-   CI: Bundlesize checks should run reliably [#&#8203;1812](ngxs/store#1812)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <[email protected]>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1401
Co-authored-by: Calciumdibromid Bot <[email protected]>
Co-committed-by: Calciumdibromid Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants