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

feat(store): add strictActionWithinNgZone runtime check #2364

Merged
merged 19 commits into from
Feb 11, 2020
Merged

feat(store): add strictActionWithinNgZone runtime check #2364

merged 19 commits into from
Feb 11, 2020

Conversation

StephenCooper
Copy link

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[X] 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?

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.

strictActionWithinNgZone: Action '[TODO] Update Todo' running outside NgZone.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

@StephenCooper
Copy link
Author

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
Copy link
Member

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(
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea

Copy link
Member

@timdeschryver timdeschryver left a 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)

@ngrxbot
Copy link
Collaborator

ngrxbot commented Feb 10, 2020

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.
Copy link
Author

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.

@brandonroberts brandonroberts merged commit 4cae255 into ngrx:master Feb 11, 2020
@brandonroberts
Copy link
Member

Thanks @StephenCooper!

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.

RFC: Add a dev-only debug check for events running outside of NgZone
4 participants