biati-digital / glightbox

Pure Javascript lightbox with mobile support. It can handle images, videos with autoplay, inline content and iframes
MIT License
2.01k stars 226 forks source link

perf(glightbox): Fixes potential memory leak with IntersectionObserver #474

Closed gingerchew closed 4 months ago

gingerchew commented 4 months ago

The build method creates an IntersectionObserver, but it is not cleaned up at any point. This means that each time an instance is opened, a new IO is created with all the previous looking for elements that no longer exist to intersect with the viewport. To prevent this, I saved the observer to the GLightbox class so it could be cleaned up using .disconnect() in the destroy method.

class GLightbox {
  private observer: IntersectionObserver;
  build() {
    // ...
    this.observer = new IntersectionObserver();
    // ...
  }

  destroy() {
    // ...
    this.observer.disconnect()
  }
}
biati-digital commented 4 months ago

Awesome, totally forgot about that. I implemented that when I was making some tests with the drag functionality (there was no plugins system yet), I still need to make some tests to see if it's required in the core or we move that to the drag plugin.

We are using Changesets to handle versions and releases. I've added more information in the CONTRIBUTING.md

Basically, just run npm run change and select the package you worked on (using the space bar), for now there's no major/minor bumps so when asked do not select anything just press enter, at the end it simply asks a summary, you can use the same text as the PR title if you want. A file will be created inside the .changeset directory, just push that file.

I'm still making tests with Changesets but seems to be working fine for now. The original plan was to use Conventional commits to automatically build and publish on npm but it definitely adds a layer of complexity and inflexibilities that could cause some problems (Graphql moved away from conventional commits/lerna to Changesets because it's easier for everyone).

gingerchew commented 4 months ago

Interesting, I hadn't heard of that before. I figured it was just some monorepo magic.

I've run the command and added a short description of the change, was that all that needed to happen?

biati-digital commented 4 months ago

Yes, that's all. Changesets it's really simple to use. I'll merge it to make sure everything it's working correctly.

biati-digital commented 4 months ago

Once merged, the GitHub Action runs and if there's changesets in the PR, Changeset will update the Changelogs and versions of packages and creates a new PR #475

When that PR is merged, the GitHub Action will build the packages and Changesets will publish to npm, create tags, etc.