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-image): fix memory leak and use more appropriate data structures for cache and listeners #10278

Merged
merged 16 commits into from
Mar 15, 2019
Merged

fix(gatsby-image): fix memory leak and use more appropriate data structures for cache and listeners #10278

merged 16 commits into from
Mar 15, 2019

Conversation

alexandernanberg
Copy link
Contributor

@alexandernanberg alexandernanberg commented Dec 4, 2018

  • Refactor to use Set and Map instead of objects and arrays.
  • Remove item from listeners when element is no longer observed (was probably a memory leak before, since we pushed items to the array with out ever removing them?).
  • Add io listener in componentDidMount instead of in the refHandler.
  • Remove io listener in componentWillUnmount.

@alexandernanberg
Copy link
Contributor Author

Hmm can someone re-run the circleci e2e test? Can't reproduce it locally 🤔

@pieh
Copy link
Contributor

pieh commented Dec 4, 2018

I did rerun, but it still happened - didn't code yet, but maybe it's not false negative if it happened 2 times in a row?

@alexandernanberg alexandernanberg changed the title fix(gatsby-image): fix memoery leak and use more appropriate data structures for cache and listeners fix(gatsby-image): fix memory leak and use more appropriate data structures for cache and listeners Dec 5, 2018
@alexandernanberg
Copy link
Contributor Author

Yeah I had forgotten to change the ref on the fixed div wrapper, e2e tests can be pretty useful 😬

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one looks good to me. Thanks for fixing this one. Memory leaks are hard to catch 👍

I added a few nitpicks 🙈

packages/gatsby-image/src/index.js Outdated Show resolved Hide resolved
packages/gatsby-image/src/index.js Outdated Show resolved Hide resolved
packages/gatsby-image/src/index.js Outdated Show resolved Hide resolved
packages/gatsby-image/src/index.js Outdated Show resolved Hide resolved
@alexandernanberg
Copy link
Contributor Author

I might need to give this another proper look after I merged master into this again, there were some issues that I thought the tests would've picked up 🤔

@wardpeet
Copy link
Contributor

I might need to give this another proper look after I merged master into this again, there were some issues that I thought the tests would've picked up 🤔

Could you elaborate which tests? And mabye add them ^^

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the createRef part as it's a breaking change, we will revisit this when we want to publish gatsby v3

@wardpeet wardpeet merged commit 9298fa3 into gatsbyjs:master Mar 15, 2019
@alexandernanberg alexandernanberg deleted the refactor/gatsby-image-listeners branch March 15, 2019 10:01
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

Successfully merging this pull request may close these issues.

None yet

3 participants