Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix($animate): silently ignore the options param if an object is not passed in #11826

Closed
wants to merge 1 commit into from
Closed

fix($animate): silently ignore the options param if an object is not passed in #11826

wants to merge 1 commit into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented May 7, 2015

Earlier versions of Angular expected a function to be passed into the
same param as the options value. This causes a nasty issue since the
internal animation code expects the options value to be an object
instead of a function. This patch adds the code to convert a function
value into an empty object when that occurs.

Closes #11713

@matsko matsko mentioned this pull request May 7, 2015
Closed
@matsko
fix($animate): silently ignore the options param if an object is not …
f51a24b 
…passed in

Earlier versions of Angular expected a function to be passed into the
same param as the options value. This causes a nasty issue since the
internal animation code expects the options value to be an object
instead of a function. This patch adds the code to convert a function
value into an empty object when that occurs.

Closes angular#11713
@Narretz
Copy link
Contributor

Narretz commented May 7, 2015

Does it make sense to remove this first thing in 1.5 and add it as a breaking change?

@Narretz
Copy link
Contributor

Narretz commented May 11, 2015

We can also add a deprecation notice for this.

Narretz
Narretz reviewed May 11, 2015
View reviewed changes
test/ngAnimate/animateSpec.js

inject(function($animate, $rootScope, $document) {
var element = $document[0].createElement('div');
element.setAttribute('id', 'crazy-man');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that left over from the previous test? The id is not used.

@Narretz Narretz added this to the 1.4.0-rc.2 milestone May 11, 2015
@petebacondarwin
Copy link
Member

It looks like this callback was removed in bf0f550

This was a breaking change for 1.3.x, and it is documented as such in the commit, since if you were relying on the callback to be called this no longer happens.

The question is whether we should let it slip through by merging this code. I feel bad doing so because, now instead of an error your callback just never gets called.

How about we add an assertion on that parameter that says that it cannot be a function then document the breaking change in the error docs for it? This would encourage people to upgrade to using promises and also highlight to them that their code is not going to work as expected.

@matsko
Copy link
Contributor Author

matsko commented May 11, 2015

Works for me. @petebacondarwin would be able to take this one on for me?

@petebacondarwin
Copy link
Member

@matsko: will do

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request May 11, 2015
@petebacondarwin
fix(ngAnimate): throw an error if a callback is passed to animate met…
9fbef99 
…hods

As of bf0f550 (released in 1.3.0) it is no longer
valid to pass a callback to the following functions: `enter`, `move`, `leave`, `addClass`,
`removeClass`, `setClass` and `animate`.

To prevent confusing error messages, this change asserts that this parameter is
not a function.

Closes angular#11826
Closes angular#11713
@petebacondarwin petebacondarwin mentioned this pull request May 11, 2015
Closed
petebacondarwin added a commit that referenced this pull request May 14, 2015
@petebacondarwin @matsko
fix(ngAnimate): throw an error if a callback is passed to animate met…
39b078b 
…hods

As of bf0f550 (released in 1.3.0) it is no longer
valid to pass a callback to the following functions: `enter`, `move`, `leave`, `addClass`,
`removeClass`, `setClass` and `animate`.

To prevent confusing error messages, this change asserts that this parameter is
not a function.

Closes #11826
Closes #11713
matsko added a commit to matsko/angular.js that referenced this pull request May 26, 2015
@matsko
fix($animate): ignore invalid option parameter values
c3e149f 
Prior to this fix there was another patch that threw an exception if the
provided options value was not of an object type. While this is correct
in terms of logic, it caused issues with plugins and tools that are
designed to work with multiple version of Angular. This fix ensures that
these plugins work since an invalid options value is ignored by
`$animate`.

Closes angular#11826
matsko added a commit to matsko/angular.js that referenced this pull request May 26, 2015
@matsko
fix($animate): ignore invalid option parameter values
f86c945 
Prior to this fix there was another patch that threw an exception if the
provided options value was not of an object type. While this is correct
in terms of logic, it caused issues with plugins and tools that are
designed to work with multiple version of Angular. This fix ensures that
these plugins work since an invalid options value is ignored by
`$animate`.

Closes angular#11826
@matsko matsko mentioned this pull request May 26, 2015
Closed
matsko added a commit that referenced this pull request May 26, 2015
@matsko
fix($animate): ignore invalid option parameter values
0623da0 
Prior to this fix there was another patch that threw an exception if the
provided options value was not of an object type. While this is correct
in terms of logic, it caused issues with plugins and tools that are
designed to work with multiple version of Angular. This fix ensures that
these plugins work since an invalid options value is ignored by
`$animate`.

Closes #11826
matsko added a commit that referenced this pull request May 26, 2015
@matsko
fix($animate): ignore invalid option parameter values
72edd4d 
Prior to this fix there was another patch that threw an exception if the
provided options value was not of an object type. While this is correct
in terms of logic, it caused issues with plugins and tools that are
designed to work with multiple version of Angular. This fix ensures that
these plugins work since an invalid options value is ignored by
`$animate`.

Closes #11826
@matsko
Copy link
Contributor Author

matsko commented May 26, 2015

Note that we had to revert this and add the 1.3 behaviour of ignoring non-object properties since UI-router, AngularStrap and other libraries still rely on it.

This commit makes the change:
72edd4d

@pdrf pdrf mentioned this pull request Jul 2, 2015
Closed
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
@petebacondarwin @netman92
fix(ngAnimate): throw an error if a callback is passed to animate met…
7aad388 
…hods

As of bf0f550 (released in 1.3.0) it is no longer
valid to pass a callback to the following functions: `enter`, `move`, `leave`, `addClass`,
`removeClass`, `setClass` and `animate`.

To prevent confusing error messages, this change asserts that this parameter is
not a function.

Closes angular#11826
Closes angular#11713
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
@matsko @netman92
fix($animate): ignore invalid option parameter values
43e5463 
Prior to this fix there was another patch that threw an exception if the
provided options value was not of an object type. While this is correct
in terms of logic, it caused issues with plugins and tools that are
designed to work with multiple version of Angular. This fix ensures that
these plugins work since an invalid options value is ignored by
`$animate`.

Closes angular#11826
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes component: ngAnimate
Projects
None yet
Milestone
1.4.0-rc.3
Development

Successfully merging this pull request may close these issues.

(bug) ngAnimate options.domOperation is not a function
4 participants
@matsko @Narretz @petebacondarwin @googlebot