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): handle mixed async scenarios for action handlers #1762

Merged
merged 8 commits into from
Jun 16, 2021

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?

There currently exists an issue where a user could mistakenly return an observable from an async method and the returned observable will not be subscribed to, and will not determine the completion event of the original action.
A similar situation exists (although not as impactful) where a promise is returned from an observable. For example, if the user used the map rxjs pipe, but declared the mapper function as async (value) => { ......

Issue Number: #1660 #1663

What is the new behavior?

The NGXS engine will inspect the inner values returned by the action handler methods and unwrap any inner observable or promise so that they are now part of the action result observable subscription.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

@markwhitfeld
Copy link
Member Author

I will probably just have to tweak the bundlesize slightly before merging... sorry @arturovt 😉

@arturovt
Copy link
Member

@markwhitfeld remember that we talked about tree-shaking flags to eliminate bug fixes from the final bundle 😁 😁

@arturovt arturovt merged commit 59cc5a2 into master Jun 16, 2021
@arturovt arturovt deleted the fix/issue-1660-handle-mix-of-promise-observable branch June 16, 2021 21:22
@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