feat(isArray): make angular.isArray support Array subclasses #15541
Conversation
|
Please consider merging this into the 1.5 branch too. |
1.5? I am even skeptical about 1.6. This is a BC, I'm afraid. |
| @@ -232,8 +232,7 @@ function isArrayLike(obj) { | |||
|
|
|||
| // NodeList objects (with `item` method) and | |||
| // other objects with suitable length characteristics are array-like | |||
| return isNumber(length) && | |||
| (length >= 0 && ((length - 1) in obj || obj instanceof Array) || typeof obj.item === 'function'); | |||
| return isNumber(length) && (length >= 0 && (length - 1) in obj || typeof obj.item === 'function'); | |||
There was a problem hiding this comment.
We are relaxing the condition a bit, but it is OK imo.
There was a problem hiding this comment.
isArray is used in a couple of lines above. So if obj instanceof Array, the function hits return on that line.
There was a problem hiding this comment.
I know. But previously there was also the restriction that isNumber(length) and length >= 0, which is gone now.
What I am saying is that isArrayLike (and anything depending on it) is slightly affected by the change in isArray.
| * | ||
| * @param {*} value Reference to check. | ||
| * @returns {boolean} True if `value` is an `Array`. | ||
| */ | ||
| var isArray = Array.isArray; | ||
| function isArray(arr) { | ||
| return arr instanceof Array || Array.isArray(arr); |
There was a problem hiding this comment.
Array.isArray(arr) is unreachable here, since by definition any array exotic object is instanceof Array.
There was a problem hiding this comment.
Not if it comes from a different context (e.g. another Window object). E.g.:
var win = window.open();
var arr = new win.Array();
console.log(arr instanceof Array); // false
console.log(Array.isArray(arr)); // true|
BTW, in anyone wanted to use an evil and totally not recommended hack for 1.5/1.6, they could surround the |
Here in PR? Or in the commit message?
I'm just patching angular.js locally :) |
I know 😉 |
9f56892 to
a787bb4
Compare
|
@Narretz done |
|
Cool! Can you please add that this change also affects |
3bbf03f to
5dd17e3
Compare
|
@Narretz done. Travis' error messages are irrelevant. |
|
LGTM. Is that okay for you @gkalpak ? |
|
I would like to see a more explicit mention of how the implementation changed/what "objects that weren't previously recognized as arrays [...] are now recognized as such". Just a brief notice that objects that prototypally inherit from |
Closes angular#15533 BREAKING CHANGE: `angular.isArray` is used by `angular.copy`, which in turn is used internally by the dirty checking logic. That's why this change affects the way objects are copied and watched by AngularJS. Objects that prototypally inherit from `Array` (e.g. MobX observable arrays, see angular#15533) weren't previously recognized as arrays, now they are. This change also affects `angular.merge`, `angular.forEach`, and `angular.equals`. Previously, `angular.isArray` was an alias for and thus worked exactly as `Array.isArray`.
5dd17e3 to
362b1cf
Compare
|
Tweaked the commit message, added some more tests and landed. |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feature
What is the current behavior? (You can also link to an open issue here)
Angular doesn't recognise objects prototypically inherited from
Array.prototypeas arrays.See #15533
What is the new behavior (if this is a feature change)?
In particular, this change gets Angular to play nicely with MobX observable arrays.
Does this PR introduce a breaking change?
Yes.
angular.isArrayis used throughout Angular. In particular, after this change,angular.copywill use the array-cloning logic for objects prototypically inherited fromArray.prototypewhereas now it uses the object-cloning logic for them. This, in turn, affects deep watchers and other parts of the framework that useangular.copy.Please check if the PR fulfills these requirements