Ensure that document functions exist before calling in ngOnDestroy() #1672

Closed
brian428 opened this Issue Dec 23, 2016 · 4 comments

Projects

None yet

3 participants

@brian428
brian428 commented Dec 23, 2016 edited

I'm submitting a ... (check one with "x")

[X ] bug report => Search github for a similar issue or PR before submitting
[ ] feature request => Please check if request is not on the roadmap already https://github.com/primefaces/primeng/wiki/Roadmap
[ ] support request => Please do not submit support request here, instead see http://forum.primefaces.org/viewforum.php?f=35

Plunkr Case (Bug Reports)
Creating a test plunk here would be difficult because the issue manifests during unit test teardown.

Current behavior
If you unit test a component that uses something like ConfirmDialog, the test must be async() and/or must wait for component to be stable, or errors will be thrown. This is due to the onNgDestroy() calls:

if(this.responsive) {
    this.documentResponsiveListener();
}

if(this.closeOnEscape && this.closable) {
    this.documentEscapeListener();
}

The problem is that if the Dialog is destroyed before the listeners are set, these calls will throw xxx is not a function errors. (In a totally separate issue, and even more annoying, these failures will cause all subsequent tests to fail, which is suspect is a Karma or Jasmine issue related to how the tests are executed.)

I know this affects ConfirmDialog, but I imagine that any component that uses document listeners like this would have the same problem.

Expected behavior
Destroying components that use document listeners should not error if the listeners have not been set. Before the functions are called during destroy, they should be checked for existence. Something like:

if(this.responsive && this.documentResponsiveListener) {
    this.documentResponsiveListener();
}

if(this.closeOnEscape && this.closable && this.documentEscapeListener) {
    this.documentEscapeListener();
}
@Chintanvpatel

@brian428 Same issue here with contextmenu component. I'm getting

undefined is not a constructor (evaluating 'this.documentClickListener()') in karma-shim.js

https://github.com/primefaces/primeng/blob/master/components/contextmenu/contextmenu.ts#L241

@cagataycivici cagataycivici self-assigned this Feb 1, 2017
@cagataycivici cagataycivici added the defect label Feb 1, 2017
@cagataycivici cagataycivici added this to the 2.0-RC3 milestone Feb 1, 2017
@cagataycivici cagataycivici added a commit that closed this issue Feb 1, 2017
@cagataycivici cagataycivici Fixed #1672 ddd175f
@brian428

Is there a reason this hasn't been included in the final 2.0 release?

@brian428

@cagataycivici Just trying to get some sort of answer about getting these fixes merged into master, because they never made it into the final 2.0. Could someone actually merge these fixes in please?

@brian428

@cagataycivici Just trying to bump this since these fixes still haven't actually been merged into the master branch. https://github.com/primefaces/primeng/blob/master/components/confirmdialog/confirmdialog.ts#L223

Can someone please get these merged in? Thanks.

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