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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions addon/modifiers/did-intersect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ export const DEFAULT_OBSERVER_OPTIONS = {};

function cleanup(instance) {
instance.unobserve.call(instance);
instance.observerManager.removeElement(instance.element);
instance.observerManager.removeElement(window);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why window cleanup here? This modifier doesn't explicitly subscribe to window at all.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I know it's weird. I'll be taking this to the interesction-observer-admin next.
I'd hope that this addon won't need these changes actually, since the issue lies on the dependency it uses.

But since I've had this experimental patch already, I decided I'd open a PR to showcase the issue. And frankly, I didn't think I'd get your attention so fast 😅

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh! Well, I may not have had a lot of time for active development, but I very much appreciate contributions and will do my best to be responsive (though I'm on vacation for a week starting Saturday, FYI). :)

I definitely think some fixes to the underlying dependency make sense in the context of this issue. If this is a workaround until you can get those fixes in, can you add a brief comment explaining why the workaround is needed, and then we can remove it once you land fixes upstream?

instance.onEnter = null;
instance.onExit = null;
instance.element = null;
}

export default class DidIntersectModifier extends Modifier {
Expand Down
5 changes: 5 additions & 0 deletions addon/services/observer-manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,9 @@ export default class ObserverManagerService extends Service {
willDestroy() {
this._admin.destroy();
}

removeElement(element) {
this._admin.elementRegistry.removeElement(element);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elementRegistry is private. Are you sure we can access it?

https://github.com/snewcomer/intersection-observer-admin/blob/master/src/index.ts#L22

this._admin.registry.removeElement(element);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is registry defined on the intersection-observer-admin class?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we perhaps need to make updates to the underlying class to expose a public method for this kind of cleanup?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll see, I'd imagine that that's what unobserve is for already, so hopefully it can hide all that

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, it seems like maybe there's some extra steps missing from the unobserve implementation in intersection-observer-admin to release these references.

}
}
48 changes: 48 additions & 0 deletions tests/integration/modifiers/real-did-intersect-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import { render } from '@ember/test-helpers';
import hbs from 'htmlbars-inline-precompile';
import sinon from 'sinon';

module(
'Integration | Modifier | did-intersect without mocks',
function (hooks) {
setupRenderingTest(hooks);

hooks.beforeEach(function () {
this.enterStub = sinon.stub();
this.exitStub = sinon.stub();
this.maxEnter = 1;
this.maxExit = 1;

this.observerManagerService = this.owner.lookup(
'service:ember-scroll-modifiers@observer-manager',
);
});

test('it removes elements from the registry on modifier cleanup', async function (assert) {
let removedElementsCount = 0;
let originalRemoveElement = this.observerManagerService.removeElement;
const observerManagerService = this.observerManagerService;

observerManagerService.removeElement = function removeElementExt(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be better accomplished with sinon.spy - it will still call the underlying function and then you can check the call count on the spy in your assertion.

...args
) {
removedElementsCount++;
originalRemoveElement.call(observerManagerService, ...args);
};
for (let index = 0; index < 50; index++) {
await render(
hbs`<div {{did-intersect onEnter=this.enterStub onExit=this.exitStub}}>
<span>Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.</span>
</div>`,
);
}

assert.strictEqual(
removedElementsCount,
49 * 2, // once for the element, once for a window object
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also assert against the underlying IntersectionObserverAdmin to ensure its registry gets cleaned up and prove that your implementation of removeElement works.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I wanted to inspect the count of elements, but the registry is a WeakMap which doesn't have a length/count method.

Copy link
Owner

@elwayman02 elwayman02 Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if you could do something clever like grab a reference to one of the elements (which are the keys to the registry, I think?) and then assert that a get() call on the WeakMap returns undefined after it's been cleaned up? We wouldn't necessarily need to assert all of them, you could set up a separate test that just renders once and make sure that one gets cleaned up. 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did think about that but then the issue is that you'd probably want to make sure that you no longer hold this reference yourself, so it'd just get funky 👯

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm thinking is that you grab the reference only in the test. The sinon spy for removeElement should be able to remember what its call arguments were, which you could use to get access to the element, and then you can assert against the registry contents from there.

);
});
},
);
Loading