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

Tooltip: this._unregisterListenersFn is not a function #2199

Closed
bygrace1986 opened this issue Mar 1, 2018 · 3 comments
Closed

Tooltip: this._unregisterListenersFn is not a function #2199

bygrace1986 opened this issue Mar 1, 2018 · 3 comments

Comments

@bygrace1986
Copy link

bygrace1986 commented Mar 1, 2018

Bug description:

If an element with the tooltip directive on it is quickly created and destroyed by a structural directive then it causes the following error:

AppComponent.html:3 ERROR TypeError: this._unregisterListenersFn is not a function
    at NgbTooltip.ngOnDestroy (tooltip.js:152)
    at callProviderLifecycles (provider.js:588)
    at callElementProvidersLifecycles (provider.js:556)
    at callLifecycleHooksChildrenFirst (provider.js:540)
    at destroyView (view.js:600)
    at callWithDebugContext (services.js:843)
    at Object.debugDestroyView [as destroyView] (services.js:382)

_unregisterListenersFn is initialized in ngOnInit so it appears that it is not being invoked before ngOnDestroy.

Link to minimally-working plunker that reproduces the issue:

https://stackblitz.com/edit/acl-directive

Version of Angular, ng-bootstrap, and Bootstrap:

Angular: 5.0.0
ng-bootstrap: 1.0.0
Bootstrap: N/A

@pkozlowski-opensource
Copy link
Member

@bygrace1986 here is a minimal plunker that confirms my initial thinking: http://plnkr.co/edit/wTdALkovBrdIFJBHoIpk?p=preview

Please notice that calling viewRef.detectChanges() (you can uncomment a line in my plunker) triggers ngOnInit.

So, it looks like creating and immediately destroying a view can result in a situation where ngOnDestroy is called without ngOnInit first.

Given all this the real question is: is it a valid use-case? I can see how you got into this situation in your implementation, but I would argue that this implementation us sub-optimal. Here is why:

  • you are creating / destroying views based on @Input's setter invocation and you will have 2 invocations potentially (like in the initial render). This is clearly wasteful as you are creating / destroying views for nothing.;
  • you are using this.view.clear(); to remove a view but this is rather "brutal" as it would destroy all the views in a given container, not only ones created by your directive.

If you fix 2 above problems in a different implementation of your acl directive you would have more robust implementation and you wouldn't run into issues with tooltips, see: https://stackblitz.com/edit/acl-directive-dpvcsx?file=app%2Facl.directive.ts

I believe that the implementation I'm proposing is simpler, more performant and correct. You see problems with tooltips since you are creating and destroying views for nothing.

I will probably add a guard to tooltip's ngOnDestroy method to protect its implementation from scenarios like this, but I would strongly suggest changing your implementation of the aclIf directive.

@bygrace1986
Copy link
Author

@pkozlowski-opensource Agreed, I realized that it wasn't optimal to create an destroy it so quickly but it illustrated the point so I left it alone. I've seen it come up with weird routing scenarios too and apparently someone found a way to do it with ngIf. I think that most scenarios where this is an issue can and should be avoided but it is still good to guard against it. As far as it being "brutal" ngIf is doing the same thing (https://github.com/angular/angular/blob/master/packages/common/src/directives/ng_if.ts) and that is the behavior that I wanted to mimic. I want to destroy all views in the container.

Thanks for taking the time to look into the issue and the feedback!

@devoto13
Copy link
Contributor

devoto13 commented Mar 2, 2018

I also have seen routing causing this issue. IIRC it was happening when redirect is triggered from the component constructor. So change detection run was creating components, and than redirect happened in one of them and it started destroying components before it got to calling ngOnInit() on them.

Happy that it is being fixed! -1 workaround in my project 😄

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

No branches or pull requests

3 participants