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

inViewportAction seems to run outside of NgZone #1284

Closed
karptonite opened this issue Feb 21, 2023 · 6 comments · Fixed by #1288, #1289 or #1290
Closed

inViewportAction seems to run outside of NgZone #1284

karptonite opened this issue Feb 21, 2023 · 6 comments · Fixed by #1288, #1289 or #1290

Comments

@karptonite
Copy link

karptonite commented Feb 21, 2023

This appears to be a regression in 15.0 from 13.0.1, although I haven't eliminated the possibility that it is something that I'm doing wrong.

I have code that looks like this:

<span
    inViewport
    (inViewportAction)="loadOnIntersection($event)"></span>

and in the component:

loadOnIntersection({ visible }: { visible: boolean }) {
    console.log(NgZone.isInAngularZone());
    // other stuff
}

With version 13.0.1, isInAngularZone() reports as true, and with version 15.0, it reports as false.

I'm doing stuff in the loadOnIntersection function that expects to be run in zone. I can explicitly do this, but I think the intention is that inViewportAction should be in zone as well.

@k3nsei
Copy link
Owner

k3nsei commented Feb 22, 2023

I think that it would be not optimal to run changes detection whenever something changes it's visibility. Especially when you have many elements that are tracked. So to have best performance I think that change detection should be run based of a results from inViewportAction and then inside it user of library would call ChangeDetectorRef.detectChanges(); when needed as explicit task.

@karptonite
Copy link
Author

karptonite commented Feb 22, 2023

I'm not an expert in NgZone and change detection. I assumed based on issue #176, that it was the intention for the loadOnIntersection event to emit inside Zone. It seemed to me that the behavior now is what the poster in issue #176 was asking for as an option. But I could definitely be misunderstanding this.

My use case isn't about change detection, but about dispatching an action to NGRX inside of NgZone, as described here: https://ngrx.io/guide/store/configuration/runtime-checks#strictactionwithinngzone.

I can always fix this at my end by calling dispatch inside of ngZone.run, but I reported this here because it seemed like there was a significant change between version 13 and 15, and that it might be a breaking change for those relying on the previous behavior. It may well be that there is no change, and I am misunderstanding it. It may also be that there is a change, and it is a good one for performance.

@k3nsei
Copy link
Owner

k3nsei commented Feb 22, 2023

@all-contributors please add @karptonite for bug

@allcontributors
Copy link
Contributor

@k3nsei

I've put up a pull request to add @karptonite! 🎉

@karptonite
Copy link
Author

Thanks! As I mentioned, this might be a negative for performance, and it might be better to either ask people to handle the events with NgZone themselves, or add an option to take it out of NgZone, as requested in #176. I'm not sure, honestly, but this should restore the previous behavior for my use case, anyway!

@k3nsei
Copy link
Owner

k3nsei commented Feb 22, 2023

The library should not expect Angular users to use advanced strategies and should work with the framework's defaults. It was also a nice way of testing automatic releasing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment