Skip to content

Memory leak? #67

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Memory leak? #67

wants to merge 5 commits into from

Conversation

brandly
Copy link

@brandly brandly commented Jul 25, 2013

If you check out memory usage on the Timeline tab in Chrome dev tools, every page transition seems to allot more memory. Occasionally, you'll see memory usage drop a bit, but it seems really inconsistent.

I'm going to dig through the code, but I wanted to see if you knew anything about this first.

@brandly
Copy link
Author

brandly commented Jul 16, 2013

I haven't found anything promising yet. You can replicate it on the demo page.

@NickyYo
Copy link

NickyYo commented Jul 23, 2013

The problem is each page load keeps creating a new page rather than going back to a page already created. This plugin is so buggy.

@ajoslin
Copy link
Owner

ajoslin commented Jul 23, 2013

Each time you leave a route, it destroys the previous scope and element,
then creates a new one.

On Tue, Jul 23, 2013 at 9:29 AM, Nick [email protected] wrote:

The problem is each page load keeps creating a new page rather than going
to back to a page already created. This plugin is so buggy.


Reply to this email directly or view it on GitHubhttps://github.com//issues/67#issuecomment-21413535
.

@brandly
Copy link
Author

brandly commented Jul 23, 2013

Yeah, it's not quite that simple @NickyYo.

@ajoslin what's the reasoning for storing currentTrans and calling .cancel() here?

currentTrans seems to actually mean lastTrans, since cancel doesn't get called until a subsequent transition, but cancel doesn't really seem to do anything.

I've removed the cancel method, the currentTrans var, and the entire changePage closure, since I think it was holding on to the dest reference and preventing the garbage collector from doing work. In the very least, it's odd that changePage is passed three values here, despite not accepting any values here.

Memory management seems to be better, so I'll send you a pull request later.

@NickyYo
Copy link

NickyYo commented Jul 24, 2013

Hmm good point there.. Think il do the same Brandly.

@ajoslin
Copy link
Owner

ajoslin commented Jul 24, 2013

Thanks so much Matt!! :-)

On Wednesday, July 24, 2013, Nick wrote:

Hmm good point there.. Think il do the same Brandly.


Reply to this email directly or view it on GitHubhttps://github.com//issues/67#issuecomment-21470546
.

...sent from my iPhone

@brandly
Copy link
Author

brandly commented Jul 25, 2013

This is the code we're currently using, and it seems to be working well.

@ryngonzalez and I both worked on the changes, so let us know what you think.

@ajoslin
Copy link
Owner

ajoslin commented Jul 26, 2013

LGTM besides that one thing. I wish we had unit tests!!! Back when I made this I didn't do tests much...

If you want to add unit tests matt you'd be king of everything :-p

But it's not very testable right now anyway.

I'll merge it after the one thing I commented :-)

@ajoslin
Copy link
Owner

ajoslin commented Jul 26, 2013

...and the purpose of cancel was if you are changing pages while a transition is going, it would immediately finish the last transition for you. But that is fixed now with the for loop you added.

@brandly
Copy link
Author

brandly commented Jul 26, 2013

Even after updating Angular, I'm still getting a Unknown provider: $sniffer error. Am I injecting it incorrectly?

@ajoslin
Copy link
Owner

ajoslin commented Jul 26, 2013

You're trying to inject $sniffer into the provider area, but the provider injection only accepts other providers.

$sniffer is a normal service not a provider, so you inject it in this.$get, where the actual factory is run.

@artworkad
Copy link

+1 👍

@artworkad
Copy link

@brandly navigation works. However the scopes inside my controllers seem to be lost when navigating. Instead of data I get just {{ data }} in my views.

@brandly
Copy link
Author

brandly commented Aug 4, 2013

Are you navigating back via $navigate.back() or the browser's back button?
There no browser history support currently.

On Sunday, August 4, 2013, ArtworkAD wrote:

Just tested, not really working. When I navigate back the scope seems to
be lost, instead of data I just get {{ field }}


Reply to this email directly or view it on GitHubhttps://github.com//pull/67#issuecomment-22070939
.

@artworkad
Copy link

@brandly nevermind works now, it was an android specific problem 😋

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.

4 participants