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): disallow nullable actions in dispatch() #2221

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

pietzschke
Copy link
Contributor

@pietzschke pietzschke commented Sep 23, 2024

In this commit, we update the dispatch() implementation to account for nullable parameters.
For example, dispatch(null) is invalid because null is not a valid action.

We provide both runtime and compile-time checks. The runtime check is the code that throws an error
when the provided argument is nullable, while the compile-time check is a type that excludes
nullable types.

Copy link

nx-cloud bot commented Sep 23, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c6aadd2. 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.

@arturovt
Copy link
Member

I adopted your change in one of the apps, but the build is now failing with TS2345: Argument of type 'unknown' is not assignable to parameter of type '{} | {}[]', indicating that this change is considered a breaking change.

@arturovt
Copy link
Member

To adopt this change in a more user-friendly manner, we should include a check guarded by ngDevMode, throwing an error immediately if actions is == null. This change will only throw runtime errors in development mode, meaning that no production applications will be affected.

@arturovt arturovt force-pushed the fix/prevent-undefined-dispatch branch from 8be71a3 to 4311fef Compare October 21, 2024 13:58
Copy link

pkg-pr-new bot commented Oct 21, 2024

Open in Stackblitz

@ngxs/form-plugin

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

@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/router-plugin

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

@ngxs/websocket-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]

commit: c6aadd2

Copy link

bundlemon bot commented Oct 21, 2024

BundleMon

Files updated (1)
Status Path Size Limits
fesm2022/ngxs-store.mjs
100.89KB (+584B +0.57%) 102KB / +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 +584B +0.44%

Groups updated (2)
Status Path Size Limits
@ngxs/store(esm2022)[gzip]
./esm2022/**/*.mjs
223.42KB (+945B +0.41%) +1%
@ngxs/store(fesm2022)[gzip]
./fesm2022/*.mjs
30.85KB (+117B +0.37%) +1%

Final result: ❌

View report in BundleMon website ➡️


Current branch size history | Target branch size history

Copy link

bundlemon bot commented Oct 21, 2024

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

Copy link

bundlemon bot commented Oct 21, 2024

BundleMon (Integration Projects)

Files updated (1)
Status Path Size Limits
Main bundles(Gzip)
hello-world-ng18/dist-integration/browser/mai
n-(hash).js
71.83KB (+57B +0.08%) +1%
Unchanged files (2)
Status Path Size Limits
Main bundles(Gzip)
hello-world-ng17/dist-integration/main.(hash)
.js
68.64KB +1%
Main bundles(Gzip)
hello-world-ng16/dist-integration/main.(hash)
.js
67.75KB +1%

Total files change +66B +0.03%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@arturovt arturovt force-pushed the fix/prevent-undefined-dispatch branch from 4311fef to 6f66858 Compare October 21, 2024 14:06
@arturovt arturovt changed the title fix: disallow undefined in dispatch of events fix(store): disallow nullable actions in dispatch() Oct 21, 2024
@arturovt arturovt force-pushed the fix/prevent-undefined-dispatch branch from f754706 to 40be833 Compare October 21, 2024 17:47
In this commit, we update the `dispatch()` implementation to account for nullable parameters.
For example, `dispatch(null)` is invalid because `null` is not a valid action.

We provide both runtime and compile-time checks. The runtime check is the code that throws an error
when the provided argument is nullable, while the compile-time check is a type that excludes
nullable types.
@arturovt arturovt force-pushed the fix/prevent-undefined-dispatch branch from 61bb1ab to 533a8f8 Compare October 23, 2024 11:01
Copy link

codeclimate bot commented Oct 23, 2024

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

The test coverage on the diff in this pull request is 100.0% (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 arturovt merged commit 7e1c703 into ngxs:master Oct 23, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants