ngx-errors icon indicating copy to clipboard operation
ngx-errors copied to clipboard

change subject.unsubscribe() to subject.complete()

Open sougiovn opened this issue 8 years ago • 9 comments

This pull request is related to a bug I found out when testing the ngx-errors to use on my project. I discovered that another user had already opened an issue #22 . The bug occurs because the BehaviorSubject is closed when ngOnDestroy is called, but don't recieve another instance when a new component is created, making it impossible to use this component with ngIf directive. Hunting down this bug I found out that the BehaviorSubject references was kind of "dirty" because it was being unsubscribed() when the ngOnDestroy from ngxerrors was being called. As the ngxerrors wasn't subscribing to it, there was no reason to unsubscribe(), to leave this reference you should complete the subject as the ngxerrors is it's manager. By completing it, all subscriptions should be already unsubscribed and the components was ready to receive other instance of BehaviorSubject when a new component is created. And passed the instantiation of the BehaviorSubject to the ngOnInit lifecycle hook.

What are you adding/fixing? Bug fix #22

Have you added tests for your changes? I tried to add, but failed. I couldn't test if the fixture.detectChanges() was triggering and error.

Will this need documentation changes?

No

Does this introduce a breaking change?

No

Other information

sougiovn avatar Sep 20 '17 01:09 sougiovn

Don't know why CI tests failed, but running yarn test locally everything passed.

sougiovn avatar Sep 20 '17 02:09 sougiovn

I've been noticing these errors when my project restarts in dev after a code change/rebuild, and I'm assuming they're related to this issue.

ObjectUnsubscribedError: object unsubscribed
    at new ObjectUnsubscribedError (http://localhost:4000/vendor/vendor.js:95600:26)
    at BehaviorSubject.__vendor../node_modules/rxjs/Subject.js.Subject.next (http://localhost:4000/vendor/vendor.js:76846:19)
    at BehaviorSubject.__vendor../node_modules/rxjs/BehaviorSubject.js.BehaviorSubject.next (http://localhost:4000/vendor/vendor.js:75893:31) at 
NgxErrorsDirective../node_modules/@ultimate/ngxerrors/src/ngxerrors.directive.js.NgxErrorsDirective.checkStatus (http://localhost:4000/build/app.js:39410:26)
    at http://localhost:4000/build/app.js:39419:19
    at ZoneDelegate.__vendor../node_modules/zone.js/dist/zone.js.ZoneDelegate.invokeTask (http://localhost:4000/vendor/vendor.js:97014:31)
    at Object.onInvokeTask (http://localhost:4000/vendor/vendor.js:36284:33)
    at ZoneDelegate.__vendor../node_modules/zone.js/dist/zone.js.ZoneDelegate.invokeTask (http://localhost:4000/vendor/vendor.js:97013:36)
    at Zone.__vendor../node_modules/zone.js/dist/zone.js.Zone.runTask (http://localhost:4000/vendor/vendor.js:96781:47)
    at __vendor../node_modules/zone.js/dist/zone.js.ZoneTask.invokeTask (http://localhost:4000/vendor/vendor.js:97088:34)"
__proto__:
Error

alignsoft avatar Oct 05 '17 17:10 alignsoft

Any update on this? I am seeing this error still when using with ngIf. We would love to use it in production soon. I know you're a busy guy @toddmotto - thank you for providing this module.

jc-white avatar Oct 09 '17 14:10 jc-white

Well, until the PR get approved I published my forked version @jarinwhite @alignsoft

https://www.npmjs.com/package/ngx-errors

sougiovn avatar Oct 17 '17 10:10 sougiovn

Any idea when this will be merged, this error is still present and when combined with *ngIf

virgil-av avatar Mar 01 '18 15:03 virgil-av

@virgil-av waiting for @toddmotto

sougiovn avatar Mar 01 '18 15:03 sougiovn

Thank you @gigioSouza until @toddmotto approves this PR I will remove ngx-error from the project as I have a few forms that relay on *ngIf to hide and show, but good job on fixing the problem so clean.

virgil-av avatar Mar 01 '18 16:03 virgil-av

@gigioSouza that package doesn't work for me, looks to contain the whole github repo.

I've set up an npm package which contains the changes in this pull request

https://www.npmjs.com/package/ngx-errors-complete

szilarddavid avatar May 08 '18 14:05 szilarddavid

I've tried using the forked version of this @szilarddavid posted, and I'm getting the exact same error.

Just so I'm clear - I'm seeing this on components where *ngIf is used to hide a div that contains an ngx-error directive, and the 'TypeError: Cannot read property 'unsubscribe' of undefined' is thrown when the containing component is destroyed.

Is that correct? Should the modified ngx-errors-complete resolve this issue?

alignsoft avatar Sep 06 '18 21:09 alignsoft