ngView cleanup order

[geddski]

@matsko I think the ngView cleanup order is incorrect. Current it's:

function cleanupLastView() {
  if(previousElement) {
    previousElement.remove();
    previousElement = null;
  }
  if(currentScope) {
    currentScope.$destroy();
    currentScope = null;
  }
  if(currentElement) {
    $animate.leave(currentElement, function() {
      previousElement = null;
    });
    previousElement = currentElement;
    currentElement = null;
  }
}

So a user lands on a page, the first view is shown, and currentElement is set. Next the user clicks on a link to go to the second view. cleanupLastView() gets called and should cleanup the first view. But previousElement is undefined still (it gets set below which is too late) and so the cleanup does not happen.

Only after navigating to a third view is previousElement defined and the cleanup starts to work.

Also it seems to me like the scope should be destroyed before the element is removed, but maybe that doesn't matter.

Correct me if I'm wrong but I think the order should be:

function cleanupLastView() {
  if(currentElement) {
    $animate.leave(currentElement, function() {
      previousElement = null;
    });
    previousElement = currentElement;
    currentElement = null;
  }
  if(currentScope) {
    currentScope.$destroy();
    currentScope = null;
  }
  if(previousElement) {
    previousElement.remove();
    previousElement = null;
  }
}

[IgorMinar]

guys, this looks like a no-brainer issue. why is it still open?

cc: @matsko, @tbosch, @jeffbcross

[IgorMinar]

I think that @geddski is wrong. the current code is not incorrect because the current view is removed via $animate.leave and previousElement.remove() serves only as a fallback in case the animation is taking too long and another animation already started removing the next view.

@matsko do we need this fallback at all? wouldn't it make it impossible to have more than one leave animation in progress? why would that be a bad thing?

[jeffbcross]

"no-brainer" he says

[geddski]

Is that the case for apps that aren't using ngAnimate?

[IgorMinar]

@jeffbcross I mean that it's an issue that can be resolved quickly, not necessarily that @geddski's proposal is right.

[IgorMinar]

@geddski the $animate.leave will work even when ngAnimate is not present.

I'm pretty sure that the previousElement.remove() is unnecessary and should be removed. Unless @matsko screams we should make it so.

[IgorMinar]

I opened a PR. @matsko please review

[IgorMinar]

here is a plunker with the demo of the current state: http://plnkr.co/edit/O3OYwOzihJgokI7gLLQ4?p=preview

[jeffbcross]

@IgorMinar our triage and planning process is flawed. We don't do a good job of reviewing issues by frequency/severity.

[geddski]

you may want to double check, I'm pretty certain it doesn't cleanup until you've visited the third view. If I'm right @IgorMinar owe's me lunch.

[IgorMinar]

@geddski you are wrong

[IgorMinar]

@matsko convinced me that my PR is wrong too, so I'm going to change it: #9355 (comment)