cvuorinen / angular1-async-filter

Angular2 async pipe implemented as Angular 1 filter to handle promises & RxJS observables
MIT License
59 stars 6 forks source link

Memory leaks? #8

Open felixfbecker opened 7 years ago

felixfbecker commented 7 years ago

Hi, stumbled across your blog post and module. I implemented the same module over a year ago - but I used a WeakMap to store promises. I see you are not, wouldn't that cause memory leaks?

https://github.com/felixfbecker/angular-async-filter

kylecordes commented 7 years ago

@felixfbecker - I am not the author of this thing, but I did feature it in the following video:

https://www.youtube.com/watch?v=XY9VPrKuJaA

I suspect the answer to your question in practice, at least for me, is @cvuorinen 's clever observation that by writing something | async(this) in the template, the async implementation gets access to the scope by which to register an $onDestroy handler and therefore stop listening at the right time.

(That's not so relevant to yours, as it is promise-only. I have not investigated whether this implementation is memory safe when not using this.)

felixfbecker commented 7 years ago

okay, so that seems to be required, which is not that nice... but still, even if they do get cleaned up on $destroy, if the scope is not destroyed it would not get garbage collected, right? Don't know if it's actually possible to have a situation where the scope lives longer than the object though.

I didn't add Observables at the time cause I didn't need it, but would have happily accepted a PR

cvuorinen commented 7 years ago

You're both correct. Currently there can be memory leak when not passing in the scope. When scope is passed in, it will get cleaned up on scope destroy. See https://github.com/cvuorinen/angular1-async-filter/issues/1 and https://github.com/cvuorinen/angular1-async-filter/pull/2

As mentioned in the above issue, I did also consider using WeakMap, but decided against it to avoid having to use a polyfill.

Maybe should detect if WeakMap is supported and use it if it is and fall back to current implementation if not?

felixfbecker commented 7 years ago

@cvuorinen

Maybe should detect if WeakMap is supported and use it if it is and fall back to current implementation if not?

at that point you are essentially using your own polyfill ;)

I wonder why you decided to publish your own module, was there anything anything that put you off from collaborating on https://github.com/felixfbecker/angular-async-filter?

kylecordes commented 7 years ago

As an interested non-author of either, I would be thrilled if these would consolidate to a single implementation that does all the things well:

cvuorinen commented 7 years ago

at that point you are essentially using your own polyfill ;)

Yes, but users of this package would not be required to install anything additional or try to figure out if they need it or not.

Don't remember exactly the reasons why I created my own package since it was over a year ago. Might have been because Observable support, or the WeakMap polyfill. The code actually evolved from a project I was working on at the time, so might be just that I already had it working and then decided to publish it to npm.

felixfbecker commented 7 years ago

@cvuorinen Well, in turn the ones who target newer browsers end up with either memory leaks or dead code.

image

Adding polyfill on the other hand is a matter of adding

<script src="https://unpkg.com/weakmap"></script>`

to your page.

@kylecordes Just checking for Symbol.Observable without definitely be the path I would take. Don't care who has ownership of this after all, but I think having this twice on npm has no benefit to users :)

kylecordes commented 7 years ago

About the easy polyfill: Most of the projects I see are in-house, behind a firewall. The applications bring in code only from local servers, not from anywhere on the web. Therefore the polyfill is not quite so easy:

There is some merit in building in the ability to operate even on older browsers.