perf(store): run change detection once for all selectors when asynchronous action has been completed #1828

arturovt · 2022-03-02T14:24:39Z

No description provided.

packages/store/tests/zone.spec.ts

packages/store/src/internal/state-stream.ts

markwhitfeld · 2022-03-09T20:17:02Z

packages/store/src/store.ts

@@ -87,7 +98,9 @@ export class Store {
   * Allow the user to subscribe to the root of the state
   */
  subscribe(fn?: (value: any) => void): Subscription {
-    return this._stateStream.pipe(leaveNgxs(this._internalExecutionStrategy)).subscribe(fn);
+    return this._selectableStateStream
+      .pipe(leaveNgxs(this._internalExecutionStrategy))


Is this still necessary since we are using the new shared stream?

This is necessary in case the Store class is subscribed outside of the Angular zone.

I thought that the new _selectableStateStream would handle ensuring that the subscribers are notified in the correct zone? But maybe there would be no guarantee for new subscriptions where we first receive the current value that already exists in the stream?
If this is the case, then would we not also have to do this if the store.select is subscribed to outside of the Angular Zone?

The _selectableStateStream cannot ensure that completely. Given this example:

constructor(store: Store, ngZone: NgZone) { ngZone.runOutsideAngular(() => { setTimeout(() => { store.subscribe(() => console.log(Zone.current)); // will log angular }, 3000); }); }

Otherwise, it'll log <root>. The _selectableStateStream is an optimization that re-enters the zone before publishReplay() so that we trigger a single tick() for all existing subscriptions.

packages/store/tests/issues/issue-933-selectors-causing-ticks.spec.ts

packages/store/src/store.ts

markwhitfeld · 2022-03-10T10:21:21Z

packages/store/src/store.ts

@@ -87,7 +98,9 @@ export class Store {
   * Allow the user to subscribe to the root of the state
   */
  subscribe(fn?: (value: any) => void): Subscription {
-    return this._stateStream.pipe(leaveNgxs(this._internalExecutionStrategy)).subscribe(fn);
+    return this._selectableStateStream
+      .pipe(leaveNgxs(this._internalExecutionStrategy))


I thought that the new _selectableStateStream would handle ensuring that the subscribers are notified in the correct zone? But maybe there would be no guarantee for new subscriptions where we first receive the current value that already exists in the stream?
If this is the case, then would we not also have to do this if the store.select is subscribed to outside of the Angular Zone?

markwhitfeld · 2022-05-21T21:51:16Z

@arturovt is this still in draft?
Is there anything you need from me to get this ready?

arturovt · 2022-05-21T21:59:20Z

Glad you're back! No, I've made is as a draft initially, but this is ready now to be merged (if all requirements are passed).

markwhitfeld

I'm happy with this. Just need to fix the failing bundlesizes ;-)
Might be worth merging master into this branch before doing this too.

…onous action has been completed

arturovt · 2022-05-21T22:22:15Z

Updated, hope it passes, this bundlesize is such annoying guy... 😸

markwhitfeld · 2022-05-21T22:52:40Z

I couldn't agree more!!! See #1764

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

arturovt commented Mar 2, 2022

View reviewed changes

packages/store/tests/zone.spec.ts Show resolved Hide resolved

splincode approved these changes Mar 2, 2022

View reviewed changes

markwhitfeld requested changes Mar 6, 2022

View reviewed changes

packages/store/src/internal/state-stream.ts Outdated Show resolved Hide resolved

arturovt force-pushed the fix/cd branch from 85530c7 to 86c23b1 Compare March 9, 2022 14:55

arturovt changed the title ~~perf(store): do not run change detection whenever the selectors emits~~ perf(store): do not run change detection whenever selectors emit Mar 9, 2022

arturovt force-pushed the fix/cd branch from 86c23b1 to 0f078f4 Compare March 9, 2022 15:58

markwhitfeld reviewed Mar 9, 2022

View reviewed changes

markwhitfeld requested changes Mar 10, 2022

View reviewed changes

arturovt force-pushed the fix/cd branch from 0f078f4 to 347b745 Compare March 10, 2022 10:49

arturovt changed the title ~~perf(store): do not run change detection whenever selectors emit~~ perf(store): run change detection once for all selectors when asynchronous action has been completed Mar 10, 2022

arturovt force-pushed the fix/cd branch from 347b745 to 7db2e2c Compare March 10, 2022 11:15

arturovt marked this pull request as ready for review May 21, 2022 21:58

arturovt requested review from kuncevic, Carniatto, poloagustin and joaqcid as code owners May 21, 2022 21:58

markwhitfeld approved these changes May 21, 2022

View reviewed changes

arturovt added 2 commits May 22, 2022 01:14

perf(store): run change detection once for all selectors when asynchr…

ffbb75f

…onous action has been completed

fix: update bundlesize stuff

c479b41

arturovt force-pushed the fix/cd branch from 7db2e2c to c479b41 Compare May 21, 2022 22:21

arturovt merged commit 6ba2ac1 into master May 21, 2022

arturovt deleted the fix/cd branch May 21, 2022 22:30

markwhitfeld added this to the v3.7.4 milestone Jun 10, 2022

markwhitfeld added the released label Jun 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(store): run change detection once for all selectors when asynchronous action has been completed #1828

perf(store): run change detection once for all selectors when asynchronous action has been completed #1828

arturovt commented Mar 2, 2022 •

edited

Loading

markwhitfeld Mar 9, 2022

arturovt Mar 9, 2022

markwhitfeld Mar 10, 2022

arturovt Mar 30, 2022 •

edited

Loading

markwhitfeld Mar 10, 2022

markwhitfeld commented May 21, 2022

arturovt commented May 21, 2022

markwhitfeld left a comment

arturovt commented May 21, 2022 •

edited

Loading

markwhitfeld commented May 21, 2022 •

edited

Loading

perf(store): run change detection once for all selectors when asynchronous action has been completed #1828

perf(store): run change detection once for all selectors when asynchronous action has been completed #1828

Conversation

arturovt commented Mar 2, 2022 • edited Loading

markwhitfeld Mar 9, 2022

Choose a reason for hiding this comment

arturovt Mar 9, 2022

Choose a reason for hiding this comment

markwhitfeld Mar 10, 2022

Choose a reason for hiding this comment

arturovt Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

markwhitfeld Mar 10, 2022

Choose a reason for hiding this comment

markwhitfeld commented May 21, 2022

arturovt commented May 21, 2022

markwhitfeld left a comment

Choose a reason for hiding this comment

arturovt commented May 21, 2022 • edited Loading

markwhitfeld commented May 21, 2022 • edited Loading

arturovt commented Mar 2, 2022 •

edited

Loading

arturovt Mar 30, 2022 •

edited

Loading

arturovt commented May 21, 2022 •

edited

Loading

markwhitfeld commented May 21, 2022 •

edited

Loading