gabrielguerrero / ngrx-traits

NGRX Traits is a library to help you compose and reuse state logic in your angular app. There is two versions, @ngrx-traits/signals supports ngrx-signals, and @ngrx-traits/{core, common} supports ngrx.
MIT License
44 stars 3 forks source link

Don't unsubscribe from response observable after first response #103

Closed Toeler closed 1 week ago

Toeler commented 1 week ago

Removes the first() operator from the withCalls response pipe. Allowing observables that emit multiple values to be returned, with an initial loading state and then no further loading state until the params trigger a new request.

Warning: Potentially breaking change, as if the response does not complete, then the default exhaustMap operator will mean that subsequent triggers are ignored. The solution is to either use switchMap or ensure that you complete.

@gabrielguerrero Keen to discuss whether exhaustMap is still the best default operator. Or whether you want different behaviour depending on whether the call is promise-returning or observable-returning? One other idea is that we could add a check that if the call returns an observable and doesn't complete after the first value is output and the mapPipe has not been specified then have it throw some kind of error/warning.

gabrielguerrero commented 1 week ago

The reason why the default is exhaushMap, is because the main cases I envision people using withCalls were for doing mutating calls to backend, and single entity get calls like load detail load user, essentially things that benefit from exhaust because blocks double clicking etc or multiple calls. I have withEntitiesLoadingCall which defaults to switchMap (and by the way has the same first problem but better to resolve in a separate issue), which I see people using more to load entities list because is more flexible, it can be combined with filtering sorting etc that do require the switchMap, because only the last call is important. I would prefer not to change the default because I think the previous reasons still apply, and if we change it, then it will really be a breaking change. I see that it could cause an issue because if the fetch obvservable doesn't completes after first value it needs to be switchMap otherwise subsequent calls will get block, the warning is not a bad idea if its not to complicated to implement probably using angular/core isDevMode, but otherwise I think is fine, I expect people that are providing an observable that returns more than one value will be digging more in the withCalls config anyway. It might be good if you add it to the withCalls docs as a warning at least.

Toeler commented 1 week ago

@gabrielguerrero Added takeUntilDestroyed and added a developer warning as well as updated the API docs.

gabrielguerrero commented 1 week ago

Approved @Toeler thanks for the great work will merge it soon

gabrielguerrero commented 1 week ago

Hey @Toeler tried to merge it, but your test are failing in the pipeline, your code seems fine to me not sure why they time out, https://github.com/gabrielguerrero/ngrx-traits/actions/runs/9670861153/job/26699479342?pr=103 I tried changing them to async await and they pass, you can see the changes I did in the patch attached testasync.patch

Toeler commented 1 week ago

Thanks. Yeah I was trying to have a think why they were dialing in CI. TBH I prefer async/await but just mind blanked when writing and thought it couldn't be done. Happy to go with your patch.

On Wed, 26 Jun 2024, 10:55 pm Gabriel Guerrero, @.***> wrote:

Hey @Toeler https://github.com/Toeler tried to merge it, but your test are failing in the pipeline, your code seems fine to me not sure why they time out,

https://github.com/gabrielguerrero/ngrx-traits/actions/runs/9670861153/job/26699479342?pr=103 I tried changing them to async await and they pass, you can see the changes I did in the patch attached testasync.patch https://github.com/user-attachments/files/15986829/testasync.patch

— Reply to this email directly, view it on GitHub https://github.com/gabrielguerrero/ngrx-traits/pull/103#issuecomment-2191406742, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM2OYNVXFZSM2AIJKD2WCTZJKM3BAVCNFSM6AAAAABJY5TG42VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJRGQYDMNZUGI . You are receiving this because you were mentioned.Message ID: @.***>

gabrielguerrero commented 1 week ago

@Toeler cool no worries apply it to your branch and push and Ill try to merge it again

gabrielguerrero commented 1 week ago

Hey @Toeler sorry to bother again one more small thing, all test are passing now thanks for that, but I notice the commit comment, we use semantic versioning so the commit comment needs to follow a format, can you amend it to be something like

feat(signals): Don't unsubscribe from response observable after first response
Fix #102

or use yarn commit to commit but you will need to git reset --soft HEAD~1

Otherwise the build doesnt calculate the version number properly I dont have write access to your branch so I cant change it, and if I do will put me as author in the commit

github-actions[bot] commented 1 week ago

:tada: This PR is included in version 17.9.0-beta.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: