omridevk / ng-keyboard-shortcuts

Dead Simple Keyboard Shortcuts Management for Angular
https://ng-keyboard-shortcuts.vercel.app/
147 stars 34 forks source link

object unsubscribed error when used with RxJS 7 #120

Closed wwarby closed 2 years ago

wwarby commented 3 years ago

When used with rxjs version 7.3.0 (but presumably 7 and upwards) I get an error thrown with the message object unsubscribed. There's nothing in the stack trace to indicate that ng-keyboard-shortcuts is the culprit, but I've discovered through trial and error that removing the <ng-keyboard-shortcuts> component from my page's HTML template makes the issue go away. I've further established that it is the shortcuts attribute which causes it, regardless of the inputs to that attribute. I know this because <ng-keyboard-shortcuts [shortcuts]="[]"></ng-keyboard-shortcuts> causes the error, but <ng-keyboard-shortcuts></ng-keyboard-shortcuts> does not.

I can also confirm that identical code on an identical version of ng-keyboard-shortcuts does not cause this error with rxjs 6.7.0 installed, so the issue is compatibility with rxjs 7 I think, which does have some breaking changes in it.

Steps to reproduce the behavior: Embed <ng-keyboard-shortcuts [shortcuts]="[]"></ng-keyboard-shortcuts> in a page within an Angular 12 app with rxjs 7.3.0 installed

The stack trace I saw isn't helpful, but here it is anyway: at ObjectUnsubscribedErrorImpl.ZoneAwareError (http://angular.local/vendor.js:105046:33) at _super (http://angular.local/vendor.js:100647:15) at new ObjectUnsubscribedErrorImpl (http://angular.local/vendor.js:100480:9) at Subject._throwIfClosed (http://angular.local/vendor.js:96594:19) at Subject._trySubscribe (http://angular.local/vendor.js:96660:14) at http://angular.local/vendor.js:96376:31 at errorContext (http://angular.local/vendor.js:100708:9) at Subject.Observable.subscribe (http://angular.local/vendor.js:96367:73) at http://angular.local/vendor.js:99332:82 at OperatorSubscriber. (http://angular.local/vendor.js:101011:28) at http://angular.local/vendor.js:96371:30 at errorContext (http://angular.local/vendor.js:100708:9) at Observable.subscribe (http://angular.local/vendor.js:96367:73) at http://angular.local/vendor.js:99143:101 at OperatorSubscriber._this._next (http://angular.local/vendor.js:97950:21)

codewithsuman commented 3 years ago

Hi @omridevk, It seems the issue is being caused due to use of tap operator https://github.com/omridevk/ng-keyboard-shortcuts/blob/11af8878662e23f602c4adab19c296a839121441/projects/ng-keyboard-shortcuts/src/lib/ng-keyboard-shortcuts.service.ts#L199-L212 and, there have been changes to tap operator in rxjs causing the issue. Passing the function instead of subject directly should fix the issue.

Can you please confirm the fix, accordingly I will raise a PR if needed. Thanks :)

cpereyra5199 commented 2 years ago

Hello is this going to be fixed by an updated version?

CramericaIndustries commented 2 years ago

and, there have been changes to tap operator in rxjs causing the issue. Passing the function instead of subject directly should fix the issue.

Can you please confirm the fix, accordingly I will raise a PR if needed. Thanks :)

Like so?

        switchMap(sequences =>
            this.keydown$.pipe(
                map(event => {
                    return {
                        event,
                        sequences
                    };
                }),
                tap(val => this.timer$.next(val))
            )
        ),

I'm not knee deep into rxjs. I do know tap() and I do know that timer$ is a Subject, but what I don't know is what tap(this.timer$) does. Does it proxy the Observable's next()/error()/complete() functions to the timer$ Subject? I always used tap with a callback function like so tap(val => console.log(val))

omridevk commented 2 years ago

It basically calls next with the value, but I think it was removed in version 7.

omridevk commented 2 years ago

Fixed in version 13.