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

[withStyles] Use WeakMap instead of Map to prevent high memory usage #230

Merged
merged 2 commits into from Sep 11, 2019

Conversation

@noratarano
Copy link
Contributor

commented Sep 11, 2019

Summary

Using WeakMap to use weak references to the keys so the cache doesn't grow out of control. This will allow those keys to get garbage collected.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap

Reviewers

@ahuth @ljharb @indiesquidge @majapw @joeuy

@ahuth
ahuth approved these changes Sep 11, 2019
Copy link

left a comment

LGTM 🤘

@ljharb
Copy link
Collaborator

left a comment

WeakMap can’t be polyfilled, so this change will break in airbnb-supported browsers. (unless those have changed drastically in 2 months)

An alternative is to use typeof to detect it, and fall back to a Map when it’s missing.

@ljharb ljharb changed the title [withStyles] Use WeapMap instead of Map to prevent high memory usage [withStyles] Use WeakMap instead of Map to prevent high memory usage Sep 11, 2019

@noratarano

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@ljharb Oh! This support table shows that we should be ok though 🤔: https://caniuse.com/#feat=mdn-javascript_builtins_weakmap

And no, I don't think the browser support policy changed in the last few months.

But anyway, this is still a good improvement. I'll make the change.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Sep 11, 2019

oh snap, fair enough. Still good to have the fallback, so it's not a breaking change :-)

@ljharb
ljharb approved these changes Sep 11, 2019
Copy link
Collaborator

left a comment

perfect, thanks <3

@noratarano

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@ljharb Thank you!

@noratarano noratarano merged commit a251fcd into master Sep 11, 2019

3 checks passed

Tidelift Dependencies checked
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@noratarano noratarano deleted the nora--use-weak-map branch Sep 11, 2019

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