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

Fix leak in slidesInView event #613

Merged

Conversation

fcasibu
Copy link
Contributor

@fcasibu fcasibu commented Nov 1, 2023

Hello first time trying out this package and I love everything about it but there's just one thing that's causing me some issues regarding the slidesInView event. I've created this Sandbox to demonstrate the issue.

To reproduce:

  1. Try not to resize the sandbox and see the counter "called" gets incremented by 1 perfectly fine
  2. Resize the sandbox
  3. Wait for the autoplay to slide automatically or slide it yourself
  4. Counter "called" gets incremented by A LOT and this shouldn't happen

I've added engine.slidesInView.destroy() in the deactivate function to fix the issue in embla-carousel package.

We can also listen to the reinit event to destroy the event but I feel like this shouldn't be done on the end user but on the package's side.

@davidjerleke
Copy link
Owner

@fcasibu thank you very much for your effort! Have you tried the changes to see if it solves the problem?

Best,
David

@fcasibu
Copy link
Contributor Author

fcasibu commented Nov 1, 2023

Hey @davidjerleke I've tested it on the playground for react and it does indeed solve the problem. Thanks 🙇

@davidjerleke
Copy link
Owner

@fcasibu thank you for your swift response. Nice work and welcome to the contributors club 🎉!

@davidjerleke davidjerleke added the bug Something isn't working label Nov 1, 2023
@davidjerleke davidjerleke linked an issue Nov 1, 2023 that may be closed by this pull request
1 task
@davidjerleke davidjerleke merged commit e6f1df3 into davidjerleke:master Nov 1, 2023
@davidjerleke davidjerleke added the resolved This issue is resolved label Nov 1, 2023
@fcasibu
Copy link
Contributor Author

fcasibu commented Nov 1, 2023

Seems like it broke a lot of tests regarding intersectionObserver should've added disconnect on the mock, sorry about that @davidjerleke

@davidjerleke
Copy link
Owner

davidjerleke commented Nov 1, 2023

@fcasibu no worries. It's just a minor thing. I can fix it. Thanks for informing me though 👍.

@davidjerleke
Copy link
Owner

@fcasibu done 🙂.

@davidjerleke
Copy link
Owner

@fcasibu I've released v8.0.0-rc15 which includes this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working resolved This issue is resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

slidesInView memory leak - Not destroyed when carousel is dismantled
2 participants