Skip to content
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

Dispatch patches with converted items on push/unshift/splice #73

Merged
merged 5 commits into from
Nov 19, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions src/can-observable-array.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,10 @@ class ObservableArray extends MixedInArray {
}

push(...items) {
convertItems(this.constructor, items);
return super.push(...items);
}

unshift(...items) {
convertItems(this.constructor, items);
return super.unshift(...items);
}

Expand Down Expand Up @@ -100,9 +98,6 @@ class ObservableArray extends MixedInArray {
// converting the arguments to the right type
for (i = 0, len = args.length - 2; i < len; i++) {
listIndex = i + 2;
// This should probably be a DefineObject but how?
args[listIndex] = convertItem(this.constructor, args[listIndex]);
//args[listIndex] = this.__type(args[listIndex], listIndex);
added.push(args[listIndex]);

// Now lets check if anything will change
Expand Down Expand Up @@ -223,17 +218,33 @@ var mutateMethods = {
}
};

const convertArgs = {
"push": function(arr, args) {
return convertItems(arr.constructor, args);
},
"unshift": function(arr, args) {
return convertItems(arr.constructor, args);
},
"splice": function(arr, args) {
return args.slice(0, 2).concat(convertItems(arr.constructor, args.slice(2)));
}
};

canReflect.eachKey(mutateMethods, function(makePatches, prop) {
const protoFn = ObservableArray.prototype[prop];
ObservableArray.prototype[prop] = function() {
const oldLength = this.length;
let args = Array.from(arguments);
if(convertArgs[prop]) {
args = convertArgs[prop](this, args);
}

// prevent `length` event from being dispatched by get/set proxy hooks
this[metaSymbol].preventSideEffects++;
const result = protoFn.apply(this, arguments);
const result = protoFn.apply(this, args);
this[metaSymbol].preventSideEffects--;

const patches = makePatches(this, Array.from(arguments));
const patches = makePatches(this, args);
dispatchLengthPatch.call(this, prop, patches, this.length, oldLength);
return result;
};
Expand Down
1 change: 1 addition & 0 deletions src/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ const helpers = {
}
}
}
return items;
}
};

Expand Down
59 changes: 59 additions & 0 deletions test/items-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const ObservableArray = require("../src/can-observable-array");
const ObservableObject = require("can-observable-object");
const type = require("can-type");
const QUnit = require("steal-qunit");
const canReflect = require("can-reflect");

module.exports = function() {
QUnit.module("ExtendedObservableArray.items");
Expand Down Expand Up @@ -133,4 +134,62 @@ module.exports = function() {
arr[0] = { num: 6 };
assert.ok(arr[0] instanceof MyObject, "Converts on an index setter");
});

QUnit.test("patches dispatched with converted members", function (assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this test?

Copy link
Member

Choose a reason for hiding this comment

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

Oh it seems not :/

Copy link
Member

Choose a reason for hiding this comment

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

I removed them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I liked the other test 😢

assert.expect(6);
const ObserveType = class extends ObservableObject {};
const TypedArray = class extends ObservableArray {
static get items() {
return type.convert(ObserveType);
}
};
const observable = new TypedArray([]);
canReflect.onPatches(observable, function(patches) {
assert.ok(patches[0].insert[0] instanceof ObserveType, "type is correct");
assert.deepEqual(patches[0].insert[0].get(), {foo: "bar"}, "props copied over");
});

observable.push({ foo: "bar" });
observable.unshift({ foo: "bar" });
observable.splice(2, 0, { foo: "bar" });
});

QUnit.test("Mutate methods patches dispatched with converted members", function (assert) {
var testSteps = [
{
method: 'push',
args: [{foo: 'bar'}]
},
{
method: 'unshift',
args: [{foo: 'bar'}]
},
{
method: 'splice',
args: [0, 0, {foo: 'bar'}]
}
];
const AType = class extends ObservableObject {};
const ATypedArray = class extends ObservableArray {
static get items() {
return type.convert(AType);
}
};

testSteps.forEach((step) => {
const { method, args, initialData = [] } = step;

const aTypedList = new ATypedArray(initialData);

canReflect.onPatches(aTypedList, function(patches) {
assert.ok(patches[0].insert[0] instanceof AType, "type is correct");
assert.deepEqual(patches[0].insert[0].get(), {foo: "bar"}, "props copied over");
assert.ok(true);
});

aTypedList[method].apply(aTypedList, args);

});

});
};