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

Events are not properly cleaned up #13

Open
firasdib opened this issue Sep 28, 2018 · 8 comments
Open

Events are not properly cleaned up #13

firasdib opened this issue Sep 28, 2018 · 8 comments

Comments

@firasdib
Copy link

You add events like this:

target.addEventListener('touchstart', initFn, hasPassive ? { passive: true } : false)

But remove them like this:

target.removeEventListener('touchstart', initFn)

When it should be:

target.removeEventListener('touchstart', initFn, hasPassive ? { passive: true } : false)
@firasdib
Copy link
Author

I've fixed the memory leaks in Impetus here: chrisbateman/impetus#41

But the author of that lib does not seem to be active anymore....

Can you use my fork instead? Or is there an alternative library?

@dy
Copy link
Owner

dy commented Sep 28, 2018

According to MDN https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener passive option does not matter on removal.

As for the impetus, if you have alternative suggestions let’s consider. Otherwise let’s wait for the merge.

@firasdib
Copy link
Author

You're right, { passive: true } does not need to be included. But if you specify capture, either by passing true/false as the third arg, or {capture: true}, you need to include it in the removal process.

While addEventListener() will let you add the same listener more than once for the same type if the options are different, the only option removeEventListener() checks is the capture/useCapture flag.

@firasdib
Copy link
Author

Bottom line: it was impetus and my fork fixes it. If the author does not merge it, should I publish it to npm and we use that instead?

@dy
Copy link
Owner

dy commented Sep 28, 2018

Let’s wait for a month. For now you can just use github as a dep. Should ping the author periodically

@firasdib
Copy link
Author

@dy In that case I need to fork both your repo and his, and use that. Not ideal tbh.

@dy
Copy link
Owner

dy commented Oct 1, 2018

@firasdib why don't you just temporarily use your github fork as "pan-zoom": "firasdib/pan-zoom" dependency? If there is no response, I will include your fork as the main entry.

@firasdib
Copy link
Author

firasdib commented Oct 1, 2018

@dy This is what I am doing now, but forks have a tendency to diverge... :-)

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

No branches or pull requests

2 participants