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(gatsby-link) Clean up IntersectionObservers to fix a memory leak #17056

Merged
merged 2 commits into from Aug 28, 2019

Conversation

@phacks
Copy link
Contributor

commented Aug 24, 2019

Description

This PR fixes a memory leak caused by IntersectionObservers not being cleaned up in case the intersection is never reached.

Many thanks to @atomiks who did a thorough investigation on the matter and came up with this solution 👏

I tested it on https://reactjs.org with the following procedure:

Here’s a comparison of before and after the fix (tested locally):

🙁Before 🙂After
Memory Leak before PR No memory leak after PR

Related Issues

Fixes #12198.

…t to prevent memory leaks

Associated issue: #12198
@phacks phacks requested a review from gatsbyjs/core as a code owner Aug 24, 2019
@KyleAMathews

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

Nice! Thanks!

Does this need done in gatsby-image as well?

@phacks

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

Don’t know yet, will have a look tomorrow!

@phacks

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

I did a bit of testing and could not reproduce the memory leak for gatsby-image. As it turns out, the leak has already been fixed by @alexandernanberg in #10278!

componentWillUnmount() {
if (this.cleanUpListeners) {
this.cleanUpListeners()
}
}

Copy link
Member

left a comment

This is great! 🎉 I feel like intersection observer should only be created once but that's not really a concern for this PR.

I added a small nit about adding a guard for this.io to make sure it exists.

packages/gatsby-link/src/index.js Show resolved Hide resolved
Copy link
Member

left a comment

Works like a charm! ❤️ thanks for fixing our bad code 😂

@wardpeet wardpeet merged commit 762506f into master Aug 28, 2019
20 checks passed
20 checks passed
Danger All good
Details
Peril All green. Nice work.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
ci/circleci: windows_unit_tests Your tests passed on CircleCI!
Details
cypress: default-group 67 tests passed in 00:28
Details
unit_tests_windows Build #20190826.14 succeeded
Details
@wardpeet wardpeet deleted the fix/clean-intersection-observers-to-fix-memory-leak branch Aug 28, 2019
waltercruz added a commit to waltercruz/gatsby that referenced this pull request Sep 8, 2019
…#17056)

* fix(gatsby-link) Clean up IntersectionObserver on componentWillUnmount to prevent memory leaks

Associated issue: gatsbyjs#12198

* Check whether IntersectionObserver exists before unobserving
@sidharthachatterjee

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Thank you once again for this! Featuring it on this month's Gazette in #17548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.