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.

Sign up for GitHub

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

Jump to bottom

Use onBecomeUnobserved for ObservableMap._hasMap garbage collection #2032

Merged
merged 1 commit into from
Jul 17, 2019
Merged

Use onBecomeUnobserved for ObservableMap._hasMap garbage collection #2032

merged 1 commit into from
Jul 17, 2019

Conversation

mayorovp
Copy link
Contributor

@mayorovp mayorovp commented Jul 4, 2019 •
edited
Loading

fixes #2031

  • Added unit tests
  • Updated changelog
  • Updated docs (either in the description of this PR as markdown, or as separate PR on the gh-pages branch. Please refer to this PR). For new functionality, at least API.md should be updated
  • Added typescript typings
  • Verified that there is no significant performance drop (npm run perf)

@mayorovp mayorovp requested a review from mweststrate July 4, 2019 20:07
@danielkcz
Copy link
Contributor

@mayorovp Please fix tests if you can. Also what about adding some additional tests to ensure we won't break in the future?

And we will need to port this to mobx4-master eventually, but looks like it shouldn't be such a problem.

@mayorovp
Copy link
Contributor Author

mayorovp commented Jul 5, 2019

@FredyC I don't know why getAtom() return has atom when no data atom is present, so I don't know how fix this error.

@mweststrate
Copy link
Member

I'll try to dive into it early next week. It might also be solvable without the atoms, if onBecomeOpbserved(this.hasMap, key, callback) is used

@mayorovp mayorovp force-pushed the fix-observablemap-leak branch from a48b5c7 to f763ace Compare July 8, 2019 11:15
@mayorovp mayorovp changed the title Use atoms instead of observables for ObservableMap.has() tracking Use onBecomeUnobserved for ObservableMap._hasMap garbage collection Jul 8, 2019
@coveralls
Copy link

coveralls commented Jul 8, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.003%) to 93.261% when pulling 152f283 on fix-observablemap-leak into e7e03e5 on master.

@mayorovp mayorovp force-pushed the fix-observablemap-leak branch from f763ace to 152f283 Compare July 9, 2019 12:57
danielkcz
danielkcz approved these changes Jul 17, 2019
View reviewed changes
Copy link
Contributor

@danielkcz danielkcz left a comment

Choose a reason for hiding this comment

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

Alright, tests are passing, so I suppose should be safe to merge. However, I don't exactly understand what's happening here, so I will leave merging to someone else.

@mweststrate mweststrate self-assigned this Jul 17, 2019
@mweststrate
Copy link
Member

Looking good to me as well, merging!

@mweststrate mweststrate merged commit bf2a60c into master Jul 17, 2019
@danielkcz danielkcz deleted the fix-observablemap-leak branch July 17, 2019 19:16
@mweststrate
Copy link
Member

Released as 4.13.0 / 5.13.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BanderasPRO BanderasPRO BanderasPRO left review comments

@danielkcz danielkcz danielkcz approved these changes

@mweststrate mweststrate Awaiting requested review from mweststrate

Assignees

@mweststrate mweststrate

Labels
🍗 enhancement
Projects
None yet
Milestone
No milestone
5 participants
@mayorovp @danielkcz @mweststrate @coveralls @BanderasPRO