-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat(store): add strictActionWithinNgZone runtime check #2364
Conversation
I am not sure about the docs test error reporting a dangling link. The link is written the same as the other runtime checks and does point to a header lower down the file. |
|
||
- [`strictStateImmutability`](#strictstateimmutability): verifies that the state isn't mutated | ||
- [`strictActionImmutability`](#strictactionimmutability): verifies that actions aren't mutated | ||
- [`strictStateSerializability`](#strictstateserializability): verifies if the state is serializable | ||
- [`strictActionSerializability`](#strictactionserializability): verifies if the actions are serializable | ||
- [`strictActionWithinNgZone`](#strictActionWithinNgZone): verifies if actions are dispatched within NgZone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should mention which checks are turned on by default, and which aren't.
) { | ||
return function(state: any, action: Action) { | ||
if (checks.action(action) && !ngCore.NgZone.isInAngularZone()) { | ||
throw new Error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we link to the docs here (and also for the realizability check)?
It can guide devs to understand what the problem is, and how to solve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my thinking behind putting the name of the check in the message.
Direct link to the docs would make that even easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea
modules/store/spec/meta-reducers/inNgZoneAssert_reducer.spec.ts
Outdated
Show resolved
Hide resolved
projects/ngrx.io/content/guide/store/configuration/runtime-checks.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few nits / comments (that can be picked up later but that I thought of while reviewing)
Co-Authored-By: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
…cks.md Co-Authored-By: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Preview docs changes for ac28681 at https://previews.ngrx.io/pr2364-ac28681/ |
|
||
- [`strictStateImmutability`](#strictstateimmutability): verifies that the state isn't mutated | ||
- [`strictActionImmutability`](#strictactionimmutability): verifies that actions aren't mutated | ||
- [`strictStateSerializability`](#strictstateserializability): verifies if the state is serializable | ||
- [`strictActionSerializability`](#strictactionserializability): verifies if the actions are serializable | ||
- [`strictActionWithinNgZone`](#strictActionWithinNgZone): verifies if actions are dispatched within NgZone | ||
|
||
These checks are all opt-in and will automatically be disabled in production builds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not entirely accurate anymore seeing that the immutability checks are on by default. Will have a go at updating this.
Thanks @StephenCooper! |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently there is no alert if an Action is running outside of NgZone which can lead to stale views as change detection will not be triggered. Would help prevent issues like
#476.
Add a runtime check that asserts dispatched actions are running within NgZone. Will be off by default and activated by the strictActionWithinNgZone flag.
Closes #2339
What is the new behavior?
An error will be thrown if an Action is running outside of NgZone and this flag is enabled.
Does this PR introduce a breaking change?
Other information