(bug) ngAnimate options.domOperation is not a function

[paveleremin]

Few days ago at production error occured:

TypeError: options.domOperation is not a function
options.domOperation();
angular...mate.js (line 2747, col 8)

https://github.com/angular/angular.js/blob/master/src/ngAnimate/animateQueue.js#L432

First I don't have this error at local machine, but after update Bower dependencies(bower update) I got it.

Here is my bower.json
...
"angular": "~1.4.0",
"angular-animate": "~1.4.0",
...

[seldary]

It happens in our app as well.
It is definitely a bug introduced in v1.4.0-rc.0.
I just downgraded angularjs to v1.4.0-beta.6, and it works fine.

[paveleremin]

@seldary, can you provide hash of the commit v1.4.0-beta.6, please?
Also, seems we must add author of it to the issue

[Narretz]

Can you try to isolate the problem and put a repro in a plnkr etc.?

[seldary]

I recreated the issue, as isolated as possible.
It uses angular-strap. I now suspect that this plugin simply doesn't support angular 1.4. I might be wrong.

It fails on the call to $digest here (angularStrap code):

    function safeDigest(scope) {
      scope.$$phase || (scope.$root && scope.$root.$$phase) || scope.$digest();
    }

[e-oz]

it's not bug of the angular-strap. I wrapped all calls of options.domOperation() by if (angular.isFunction(options.domOperation)) check and now all my e2e tests are green again. So I think options.domOperation is not declared somewhere.

[Narretz]

The API for animations has changed a bit, so it's alos possible that angular-strap needs to change something.

[paveleremin]

@mgcrea what do you think about that?

[mgcrea]

I've not investigated this issue due to lack of time, but I know that I'd like to keep 1.2+ backward compatibility for angular-strap. I'm quite confident we will find a suitable workaround for the v1.4 release, even if this issue does make it to the final release. Though it looks like this could be handled on the ngAnimate side, I would understand them not patching it since it's not a patch release and things are expected to break, I'm fine with the idea of having the burden of retro-compat carried by the libs if it can lead to a cleaner core.

Ideally we should try to come up with a simple failing test. Had glimpse at the changelog, it looks like the issue might come from c8700f0, especially the new auto-digest on chained promise breaking change.

For reference, AngularStrap uses this code for its tooltip (and therefore all derivatives), the choice was historically made to manually manage local-only digests on the tooltip scope during show/hide events to prevent triggering full root digests on every tooltip-enabled element hovering.

          // Hide any existing tipElement
          if(tipElement) destroyTipElement();
          // Fetch a cloned element linked from template
          tipScope = $tooltip.$scope.$new();
          tipElement = $tooltip.$element = tipLinker(tipScope, function(clonedElement, scope) {});

          // Set the initial positioning.  Make the tooltip invisible
          // so IE doesn't try to focus on it off screen.
          tipElement.css({top: '-9999px', left: '-9999px', display: 'block', visibility: 'hidden'});

          // Append the element, without any animations.  If we append
          // using $animate.enter, some of the animations cause the placement
          // to be off due to the transforms.
          after ? after.after(tipElement) : parent.prepend(tipElement);

          $tooltip.$isShown = scope.$isShown = true;
          safeDigest(scope);

          // Now, apply placement
          $tooltip.$applyPlacement();

          // Once placed, animate it.
          // Support v1.3+ $animate
          // https://github.com/angular/angular.js/commit/bf0f5502b1bbfddc5cdd2f138efd9188b8c652a9
          var promise = $animate.enter(tipElement, parent, after, enterAnimateCallback);
          if(promise && promise.then) promise.then(enterAnimateCallback);
          safeDigest(scope);

          $$rAF(function () {
            // Once the tooltip is placed and the animation starts, make the tooltip visible
            if(tipElement) tipElement.css({visibility: 'visible'});
          });

A potential fix for v1.4 might be something like:

          if(promise && promise.then) promise.then(enterAnimateCallback);
          else safeDigest(scope);

But that would break v1.3.

I'll try to investigate further in the coming days.

[matsko]

The bug has nothing to do with digests. The issue is that there is a function being passed in to the final parameter of enter:

var promise = $animate.enter(tipElement, parent, after, enterAnimateCallback);

So towards the end of the close function inside of ngAnimate we get the following value for the options value when console.log(options) is called:

function enterAnimateCallback() {
  scope.$emit(options.prefixEvent + '.show', $tooltip);
}

Long story short, the final param needs to be omitted for 1.3+ (due to promises). So @mgcrea do you have a clean way of doing this? We could patch ngAnimate to ignore non-object params for the 4th parameter but you would still be doing this in 1.3.

[e-oz]

Maybe not only angular-strap doing it, if it was valid in previous versions... Maybe it's the breaking change that should be announced. Or @matsko could make it non-breaking by additional check.

[matsko]

In this situation it's better to have the 1.4 $animate code do the checking: #11826

[mgcrea]

Fixed in angular-strap by mgcrea/angular-strap@afd4fc7, thanks @matsko for the bug insights.