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

RFC: Add a dev-only debug check for events running outside of NgZone #2339

Closed
StephenCooper opened this issue Jan 28, 2020 · 4 comments · Fixed by #2364
Closed

RFC: Add a dev-only debug check for events running outside of NgZone #2339

StephenCooper opened this issue Jan 28, 2020 · 4 comments · Fixed by #2364
Assignees
Labels
community watch Someone from the community is working this issue/PR enhancement Project: Store

Comments

@StephenCooper
Copy link

Describe any alternatives/workarounds you're currently using

I have just experience a change detection bug where an event was being triggered in such a way that it was running outside of NgZone. The event would trigger actions and update the store but the template would not be updated. The bug only surfaced as a setInterval was removed which had the effect of ensuring ChangeDetection was consistently run.

Adding the following meta reducer immediately highlighted the problem code.

//NgRx Meta Reducer to assert always in NgZone
export function assertInNgZoneMetaReducer(reducer) {
    return function(state: State, action: Action): State {
        NgZone.assertInAngularZone();
        return reducer(state, action);
    };
}

Having a built-in check that mimics this behaviour could be extremely valuable at catching issues in development. I believe it would have helped catch some of the errors reported in #476.

Other information:

I understand that there will be cases where this is not a valid check as developers can explicitly choose to run code outside of ngZone so this would have to be configurable. However, my assumption would be that for the majority of apps an event running outside of ngZone is a red flag and not intentional.

If accepted, I would be willing to submit a PR for this feature

[X] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

@timdeschryver
Copy link
Member

Will you create a PR for this @StephenCooper ?

@StephenCooper
Copy link
Author

I can take a look and see if I can! I haven't looked under the hood yet.

@brandonroberts brandonroberts added the community watch Someone from the community is working this issue/PR label Jan 30, 2020
@StephenCooper
Copy link
Author

Trying to decide on the default status of this runtime check.

Defaulting on would mean higher uptake of this feature and potentially help catch more issues. My assumption would be that 'most' apps are written without an explicit need to run events outside of NgZone, meaning any events this catches would be of use to users.

On the flip side defaulting on would cause errors to start being thrown for apps that do explicitly run events outside of NgZone and require toggling this runtime check off.

Is there any precedence for making this choice?

@timdeschryver
Copy link
Member

@StephenCooper the default can be be off

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community watch Someone from the community is working this issue/PR enhancement Project: Store
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants