search

elwayman02 / ember-scroll-modifiers

Scroll-based Modifiers for Ember.js Applications
https://ember-scroll-modifiers.jhawk.co/
MIT License
21 stars 11 forks source link

DRAFT: feat(memory-leak): add elements cleanup for did-intersect modifier #1078

Closed BobrImperator closed 2 months ago

BobrImperator commented 2 months ago

Our client has been experiencing severe memory leaks. Which did-intersect was partially responsible for. Currently I'm in the process of porting our monkey patches and documenting the issue. We've also found ember-in-viewport to be causing these, which in turn is using intersection-observer-admin just like this addon.

Applying an identical fixes to both addons has fixed our problems.

With fix: image

Without fix: image

elwayman02 commented 2 months ago

Thanks for identifying this issue, I'd love to get it fixed. I left a few comments, and it looks like there are a few failing tests as well. Feel free to ping me once you have updates and I'll be happy to take another pass at it!

elwayman02 commented 2 months ago

Does this fix in the underlying dependency help you at all in the context of this PR, if it lands?

https://github.com/snewcomer/intersection-observer-admin/pull/49

BobrImperator commented 2 months ago

That could be it, yeah

elwayman02 commented 2 months ago

Alright, I'll see if I can poke Scott about getting that PR landed. If you need additional fixes, feel free to open a PR in that repo as well and I'll tell him to keep an eye out for it.

BobrImperator commented 2 months ago

FYI the current master of intersection-observer-admin appears to have fixed the issue with DidIntersect and it's no longer leaking.

For ember-in-viewport I had to apply an additional fix found on my fork here. Not everything there is needed and it also has changes to package.json to make it installable with a url. Additionally it doesn't fix the issues entirely but it makes it a lot less severe. I'll post more updates here once I find more issues and fixes.

BobrImperator commented 2 months ago

Closing since intersection-observer-admin 0.3.4 has been released :tada:

https://github.com/snewcomer/intersection-observer-admin/releases/tag/v0.3.4-intersection-observer-admin

elwayman02 commented 2 months ago

That's great! If you want to open a PR forcing that as the minimum version to bring in the bug fix, I'd be happy to have it. I know we're due for a new release, I've just been pretty busy, but at least you'll be unblocked on your end by just bumping the version in your own app.

elwayman02 commented 3 weeks ago

@BobrImperator v7.2.0 contains the bumped version of intersection-observer-admin as a required dependency. Thanks again for bringing this up!