feat(store-devtools): provide the ability to connect extension outside of Angular zone

[arturovt]

This commit updates the extension connection setup and wraps it with runOutsideAngular to prevent running change detection too frequently. This ensures that change detection is only triggered when all asynchronous actions have been processed.

Closes #3839

[netlify]

✅ Deploy Preview for ngrx-io ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	62a86bf
🔍 Latest deploy log	https://app.netlify.com/sites/ngrx-io/deploys/64c3e1dae80a2c00081bce95
😎 Deploy Preview	https://deploy-preview-3970--ngrx-io.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[timdeschryver]

It seems like with this change importing an existing state throws an error.
This should be resolved first, or this should be added to the docs.

reducer.ts:75  ERROR Error: Action '[Auth/API] Login Redirect' running outside NgZone. https://ngrx.io/guide/store/configuration/runtime-checks#strictactionwithinngzone
    at inNgZoneAssert_reducer.ts:11:13
    at serialization_reducer.ts:24:23
    at immutability_reducer.ts:11:23
    at utils.ts:139:14
    at computeNextEntry (reducer.ts:72:17)
    at recomputeStates (reducer.ts:122:9)
    at reducer.ts:533:24
    at StoreDevtools.liftedStateSubscription.liftedAction$.pipe.state (devtools.ts:96:38)
    at scanInternals.js:14:21
    at OperatorSubscriber._this._next (OperatorSubscribe
[state.txt](https://github.com/ngrx/platform/files/12147326/state.txt)
r.js:33:21)
handleError @ core.mjs:10072
computeNextEntry @ reducer.ts:75
recomputeStates @ reducer.ts:122
(anonymous) @ reducer.ts:533
StoreDev
[state.txt](https://github.com/ngrx/platform/files/12147295/state.txt)
tools.liftedStateSubscription.liftedAction$.pipe.state @ devtools.ts:96

You can reproduce this by running the example app and importing my export.

npx nx serve example-app

[arturovt]

@timdeschryver thanks for checking, I haven't imported the state and haven't checked that case, I'll have a look. It actually should keep functioning seamlessly w/o breaking changes, so I'd fix it.

[timdeschryver]

LGTM.
@arturovt could this change an existing's app behavior (does this have any caveats to it)?

[arturovt]

@timdeschryver I'll post my thoughts aloud so we can discuss the concerns which I think about:

• this change may cause breaking issues in unit tests that implicitly depend on a specific number and sequence of change detections to pass their assertions. However, I am not sure if someone is unit testing the Redux plugin, which is available only in the browser. This means that we might eliminate the reason for this to be a breaking change.
• the DevtoolsExtension (which has the createChangesObservable method) is not exposed publicly to end users through the NgRx API, which means I'm not sure if someone would explicitly subscribe to createChangesObservable and be affected by the fact that subscriber events would be coming from outside the Angular zone.
• the StoreDevtools is exposed publicly. Its public properties, which may be read by end users, are liftedState and state. The state basically pipes the liftedState. The liftedState is obtained from liftedStateSubject.asObservable(), where liftedStateSubject always calls its next within the Angular zone, since we call enterNgZone() previously.

Wdyt?

[timdeschryver]

@arturovt thanks for the elaborate answer.

• I would assume no projects are using devtools to unit test their components (I don't think this will break e2e tests)
• I also assume no projects are interacting with the devtools within their "user code".

The question was more about if this change could break existing application because the result of this change is that it won't trigger the CD as much as before.
But, that's probably not the case, since most of the apps turn devtools off in production.

The other thing I was thinking about are applications that don't rely on NgZone.

[arturovt]

Apps which have the NgZone noopped won’t be affected since NgZone class is substituted with NoopNgZone which does nothing (just invokes callbacks directly).

[brandonroberts]

I do think that there is a possibility of this being an unexpected change in behavior from those who are including the Devtools in production, which I know there are cases of this out there.

That being said, we always recommend not doing this, so we could be covered in that case.

[arturovt]

@brandonroberts following what you said, can we also have a console.warn for production that logs smth like "Plugin is not recommended to be used in production"?

[brandonroberts]

I don't think that's necessary, but we could add that to the docs to be more clear.

The other option would be to have this behind a flag, such as:

provideStoreDevtools({
  ngZone: 'enabled' | 'noop'
});

And have it default to enabled for now, and you can change it to noop. In the future, we flip it to noop by default. That way, we keep existing behavior as-is for now, while allowing the new behavior going forward.

[timdeschryver]

I like that suggestion @brandonroberts 👍

[arturovt]

Alright, so I should re-work the changes and add a new config property?

[brandonroberts]

@arturovt yes

[markostanimirovic]

LGTM 👍 Thanks @arturovt!

[timdeschryver]

Thanks @arturovt !

[alex-okrushko]

Thanks for the fix @arturovt !
I ran into this issue myself, and when debugging I noticed it led back to ngrx/store-devtools 😱

@brandonroberts @markostanimirovic @timdeschryver I think it would make sense to have runOutsideAngular be enabled by default. I know it's a "change in behavior" but it's fixing something that should not have been there to begin with - app shouldn't rely on arbitrary change detection triggers outside of the app.

[brandonroberts]

Thanks for the fix @arturovt ! I ran into this issue myself, and when debugging I noticed it led back to ngrx/store-devtools 😱

@brandonroberts @markostanimirovic @timdeschryver I think it would make sense to have runOutsideAngular be enabled by default. I know it's a "change in behavior" but it's fixing something that should not have been there to begin with - app shouldn't rely on arbitrary change detection triggers outside of the app.

Agreed. We can possibly land this change as part of the 17.x release and do a migration for existing apps also to keep the current behavior.

[markostanimirovic]

Thanks for the fix @arturovt ! I ran into this issue myself, and when debugging I noticed it led back to ngrx/store-devtools 😱

@brandonroberts @markostanimirovic @timdeschryver I think it would make sense to have runOutsideAngular be enabled by default. I know it's a "change in behavior" but it's fixing something that should not have been there to begin with - app shouldn't rely on arbitrary change detection triggers outside of the app.

Sounds good to me 👍