fix(store): prevent writing to state once action handler is unsubscribed #2231

arturovt · 2024-10-18T17:17:20Z

In this commit, we update the implementation of action invocation. The key addition is that
we now prevent writing to the state whenever an action handler is unsubscribed (completed or
cancelled). Since we have a "unique" state context object for each action being invoked, we
can set its setState and patchState functions to no-ops, essentially making them do
nothing when invoked.

nx-cloud · 2024-10-18T17:19:33Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b2b3a97. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 4 targets

Sent with 💌 from NxCloud.

pkg-pr-new · 2024-10-18T17:19:58Z

Open in Stackblitz

@ngxs/devtools-plugin

yarn add https://pkg.pr.new/@ngxs/[email protected]

@ngxs/hmr-plugin

yarn add https://pkg.pr.new/@ngxs/[email protected]

@ngxs/form-plugin

yarn add https://pkg.pr.new/@ngxs/[email protected]

@ngxs/router-plugin

yarn add https://pkg.pr.new/@ngxs/[email protected]

@ngxs/storage-plugin

yarn add https://pkg.pr.new/@ngxs/[email protected]

@ngxs/store

yarn add https://pkg.pr.new/@ngxs/[email protected]

@ngxs/websocket-plugin

yarn add https://pkg.pr.new/@ngxs/[email protected]

commit: b2b3a97

bundlemon · 2024-10-18T17:23:20Z

BundleMon

Files updated (1)

Status	Path	Size	Limits
❌	fesm2022/ngxs-store.mjs	102.45KB (+1.56KB +1.55%)	103KB / +0.5%

Unchanged files (5)

Status	Path	Size	Limits
✅	fesm2022/ngxs-store-internals.mjs	11.64KB	13KB / +0.5%
✅	fesm2022/ngxs-store-internals-testing.mjs	6.83KB	7KB / +0.5%
✅	fesm2022/ngxs-store-operators.mjs	6.22KB	7KB / +0.5%
✅	fesm2022/ngxs-store-plugins.mjs	2.04KB	3KB / +0.5%
✅	fesm2022/ngxs-store-experimental.mjs	1.4KB	2KB / +0.5%

Total files change +1.56KB +1.21%

Groups updated (2)

Status	Path	Size	Limits
✅	@ngxs/store(esm2022)[gzip] _{./esm2022/*/.mjs}	224.5KB (+1.08KB +0.48%)	+1%
❌	@ngxs/store(fesm2022)[gzip] _{./fesm2022/*.mjs}	31.24KB (+402B +1.27%)	+1%

Final result: ❌

View report in BundleMon website ➡️

Current branch size history | Target branch size history

bundlemon · 2024-10-18T17:23:22Z

BundleMon (NGXS Plugins)

Unchanged files (9)

Status	Path	Size	Limits
✅	Plugins(fesm2022)[gzip] _{storage-plugin/fesm2022/ngxs-storage-plugin.m js}	4.15KB	+0.5%
✅	Plugins(fesm2022)[gzip] _{router-plugin/fesm2022/ngxs-router-plugin.mjs}	3.2KB	+0.5%
✅	Plugins(fesm2022)[gzip] _{websocket-plugin/fesm2022/ngxs-websocket-plug in.mjs}	2.64KB	+0.5%
✅	Plugins(fesm2022)[gzip] _{hmr-plugin/fesm2022/ngxs-hmr-plugin.mjs}	2.61KB	+0.5%
✅	Plugins(fesm2022)[gzip] _{form-plugin/fesm2022/ngxs-form-plugin.mjs}	2.59KB	+0.5%
✅	Plugins(fesm2022)[gzip] _{devtools-plugin/fesm2022/ngxs-devtools-plugin .mjs}	2.23KB	+0.5%
✅	Plugins(fesm2022)[gzip] _{logger-plugin/fesm2022/ngxs-logger-plugin.mjs}	2.03KB	+0.5%
✅	Plugins(fesm2022)[gzip] _{storage-plugin/fesm2022/ngxs-storage-plugin-i nternals.mjs}	875B	+0.5%
✅	Plugins(fesm2022)[gzip] _{router-plugin/fesm2022/ngxs-router-plugin-int ernals.mjs}	411B	+0.5%

No change in files bundle size

Unchanged groups (1)

Status	Path	Size	Limits
✅	All Plugins(fesm2022)[gzip] _{./-plugin/fesm2022/.mjs}	20.71KB	+0.5%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

bundlemon · 2024-10-18T17:23:24Z

BundleMon (Integration Projects)

Files updated (3)

Status	Path	Size	Limits
✅	Main bundles(Gzip) _{hello-world-ng17/dist-integration/main.(hash) .js}	68.69KB (+49B +0.07%)	+1%
✅	Main bundles(Gzip) _{hello-world-ng16/dist-integration/main.(hash) .js}	67.8KB (+46B +0.07%)	+1%
✅	Main bundles(Gzip) _{hello-world-ng18/dist-integration/browser/mai n-(hash).js}	71.87KB (+40B +0.05%)	+1%

Total files change +135B +0.06%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

markwhitfeld · 2024-10-21T12:31:30Z

packages/store/tests/issues/canceling-promises.spec.ts

+    @Action(MyActionAwait, { cancelUncompleted: true })
+    async handleActionAwait() {
+      recorder.push('before promise await ready');
+      await promiseAwaitReady;
+      recorder.push('after promise await ready');
+    }
+
+    @Action(MyActionThen, { cancelUncompleted: true })
+    handleActionThen() {
+      recorder.push('before promise then ready');
+      return promiseThenReady.then(() => {
+        recorder.push('after promise then ready');
+      });


What are we testing here?
There isn't anything that checks that operations on the state are nooped after cancellation.

markwhitfeld · 2024-10-21T12:59:38Z

@arturovt I wasn't meaning to remove that test file, but rather to add tests that showcase the cancellation behavior by writing to the state. Potentially the action could write the log entries to the state as well as the recorder.
Then the normal test will show that they are the same, but then when you dispatch an action that results in cancellation, then you will see that the state doesn't receive the writes.
You might want to add an id to the action so that the triggering action is apparent in the logs.

PS. I view the "don't write to state after unsubscribe" as a different set of tests to the cancellation tests (even though they use the same mechanism).

markwhitfeld

Looking good.
Please add a description to the PR, and a line to the changelog 🙏

In this commit, we update the implementation of action invocation. The key addition is that we now prevent writing to the state whenever an action handler is unsubscribed (completed or cancelled). Since we have a "unique" state context object for each action being invoked, we can set its `setState` and `patchState` functions to no-ops, essentially making them do nothing when invoked.

codeclimate · 2024-10-25T09:38:24Z

Code Climate has analyzed commit b2b3a97 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 91.6% (50% is the threshold).

This pull request will bring the total coverage in the repository to 95.4% (0.0% change).

View more on Code Climate.

arturovt force-pushed the fix/prevent-ctx-writes branch 3 times, most recently from 9cf8695 to 5999dc0 Compare October 20, 2024 22:42

markwhitfeld reviewed Oct 21, 2024

View reviewed changes

arturovt force-pushed the fix/prevent-ctx-writes branch from c14cad5 to f4de279 Compare October 24, 2024 00:07

markwhitfeld reviewed Oct 25, 2024

View reviewed changes

arturovt force-pushed the fix/prevent-ctx-writes branch from f4de279 to b2b3a97 Compare October 25, 2024 09:34

arturovt marked this pull request as ready for review October 25, 2024 09:35

arturovt requested review from kuncevic, Carniatto, poloagustin, joaqcid and splincode as code owners October 25, 2024 09:35

arturovt merged commit f22f4cb into master Oct 25, 2024
18 checks passed

arturovt deleted the fix/prevent-ctx-writes branch October 25, 2024 09:41

janpapenbrock mentioned this pull request Dec 23, 2024

🐞[BUG]: v18.1.5 / v18.1.6 silently break some state actions without error or logs #2286

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(store): prevent writing to state once action handler is unsubscribed #2231

fix(store): prevent writing to state once action handler is unsubscribed #2231

arturovt commented Oct 18, 2024 •

edited

Loading

nx-cloud bot commented Oct 18, 2024 •

edited

Loading

pkg-pr-new bot commented Oct 18, 2024 •

edited

Loading

bundlemon bot commented Oct 18, 2024 •

edited

Loading

bundlemon bot commented Oct 18, 2024 •

edited

Loading

bundlemon bot commented Oct 18, 2024 •

edited

Loading

markwhitfeld Oct 21, 2024

markwhitfeld commented Oct 21, 2024

markwhitfeld left a comment

codeclimate bot commented Oct 25, 2024

fix(store): prevent writing to state once action handler is unsubscribed #2231

fix(store): prevent writing to state once action handler is unsubscribed #2231

Conversation

arturovt commented Oct 18, 2024 • edited Loading

nx-cloud bot commented Oct 18, 2024 • edited Loading

☁️ Nx Cloud Report

pkg-pr-new bot commented Oct 18, 2024 • edited Loading

bundlemon bot commented Oct 18, 2024 • edited Loading

BundleMon

bundlemon bot commented Oct 18, 2024 • edited Loading

BundleMon (NGXS Plugins)

bundlemon bot commented Oct 18, 2024 • edited Loading

BundleMon (Integration Projects)

markwhitfeld Oct 21, 2024

Choose a reason for hiding this comment

markwhitfeld commented Oct 21, 2024

markwhitfeld left a comment

Choose a reason for hiding this comment

codeclimate bot commented Oct 25, 2024

arturovt commented Oct 18, 2024 •

edited

Loading

nx-cloud bot commented Oct 18, 2024 •

edited

Loading

pkg-pr-new bot commented Oct 18, 2024 •

edited

Loading

bundlemon bot commented Oct 18, 2024 •

edited

Loading

bundlemon bot commented Oct 18, 2024 •

edited

Loading

bundlemon bot commented Oct 18, 2024 •

edited

Loading