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

Remove seemingly unneeded code - 0.27.11-beta.0 #143

Merged
merged 1 commit into from
Oct 21, 2020
Merged

Conversation

luwes
Copy link
Owner

@luwes luwes commented Oct 20, 2020

Related to #142

Since the below block is added here fixing a memory leak it looks like the logic to pick off fresh children is not needed anymore.

if (o._runObservers) {
o._runObservers.delete(update);
}

Still kinda puzzled that it wouldn't result in any bugs. e.g
#20 (comment)

@luwes luwes self-assigned this Oct 20, 2020
@codecov
Copy link

codecov bot commented Oct 20, 2020

Codecov Report

Merging #143 into master will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   97.14%   97.08%   -0.06%     
==========================================
  Files          20       19       -1     
  Lines         560      549      -11     
==========================================
- Hits          544      533      -11     
  Misses         16       16              
Impacted Files Coverage Δ
packages/sinuous/observable/src/observable.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80b08b1...e351892. Read the comment docs.

@nettyso
Copy link
Contributor

nettyso commented Oct 20, 2020

Have you tested for memory leaks? It's the only place in the code that deletes from the run list

@luwes luwes changed the title Remove seemingly unneeded code Remove seemingly unneeded code - 0.27.11-beta.0 Oct 20, 2020
@nettyso
Copy link
Contributor

nettyso commented Oct 20, 2020

Oh actually... hmm. I mean the run list is replaced via data._runObservers = new Set() so I guess that does the job?

@luwes
Copy link
Owner Author

luwes commented Oct 20, 2020

yup that clears the set attached to the observable, and this snippet in _unsubscribe clears out the update functions.

if (o._runObservers) {
o._runObservers.delete(update);
}

I published a beta version to check with some examples.

@luwes luwes merged commit 9fda9db into master Oct 21, 2020
@luwes luwes deleted the optimize-observable branch October 21, 2020 01:17
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.

2 participants