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

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

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

gingerchew
Copy link
Collaborator

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
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Owner

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 biati-digital merged commit 2748089 into v4-beta Mar 29, 2024
1 check passed
@biati-digital
Copy link
Owner

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.

@gingerchew gingerchew deleted the potential-memory-leak-fix branch March 29, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants