destroyWith in ns.Model #149

Merged
merged 3 commits into from Aug 26, 2013

Projects

None yet

4 participants

@yanann11

No description provided.

@edoroshenko

Договорились голосом, что делаем по-другому. У модели будет метод destroyWith, который на входе получает модель и подписывается на её destroy

@edoroshenko edoroshenko commented on an outdated diff Aug 26, 2013
src/ns.model.js
@@ -91,6 +91,32 @@ ns.Model.prototype._bindEvents = function() {
};
/**
+ * Модель должна удалиться вместе с переданными моделями
+ * @param {Model| Array} models - модель или массив моделей
+ */
+ns.Model.prototype.destroyWith = function(models) {
+ if (!models) {
+ return;
@edoroshenko
edoroshenko Aug 26, 2013

Это сокрытие ошибки. Либо не проверяй, либо кидай exception

@edoroshenko edoroshenko commented on an outdated diff Aug 26, 2013
src/ns.model.js
+ * Модель должна удалиться вместе с переданными моделями
+ * @param {Model| Array} models - модель или массив моделей
+ */
+ns.Model.prototype.destroyWith = function(models) {
+ if (!models) {
+ return;
+ }
+
+ if (!Array.isArray(models)) {
+ models = [models];
+ }
+
+ for (var i in models) {
+ var model = models[i];
+ if (!model || !(model instanceof ns.Model)) {
+ throw new Error("[ns.Model] Item of linkedModels is not ns.Model ");
@edoroshenko
edoroshenko Aug 26, 2013

неправильный текст ошибки

@edoroshenko edoroshenko commented on an outdated diff Aug 26, 2013
src/ns.model.js
@@ -91,6 +91,32 @@ ns.Model.prototype._bindEvents = function() {
};
/**
+ * Модель должна удалиться вместе с переданными моделями
+ * @param {Model| Array} models - модель или массив моделей
+ */
+ns.Model.prototype.destroyWith = function(models) {
+ if (!models) {
+ return;
+ }
+
+ if (!Array.isArray(models)) {
+ models = [models];
+ }
+
+ for (var i in models) {
@edoroshenko
edoroshenko Aug 26, 2013

ты итерируешь массив, как объект. Используй forEach() или for

@edoroshenko edoroshenko commented on an outdated diff Aug 26, 2013
src/ns.model.js
@@ -91,6 +91,32 @@ ns.Model.prototype._bindEvents = function() {
};
/**
+ * Модель должна удалиться вместе с переданными моделями
+ * @param {Model| Array} models - модель или массив моделей
+ */
+ns.Model.prototype.destroyWith = function(models) {
+ if (!models) {
+ return;
+ }
+
+ if (!Array.isArray(models)) {
+ models = [models];
+ }
+
+ for (var i in models) {
+ var model = models[i];
+ if (!model || !(model instanceof ns.Model)) {
@edoroshenko
edoroshenko Aug 26, 2013
  1. Проверка !model не нужна
  2. Давай от идти от позитива, типа если model instanceof ns.Model, то всё делаем. else - пшёл вон
@edoroshenko edoroshenko commented on an outdated diff Aug 26, 2013
test/spec/ns.model.js
+ }
+ });
+ this.model1 = ns.Model.get('model1', {id: 1}).setData({key: 1});
+
+ ns.Model.define('model2', {
+ params: {
+ id: null
+ }
+ });
+ this.model2 = ns.Model.get('model2', {id: 1}).setData({key: 1});
+
+ this.model2.destroyWith(this.model1);
+
+ ns.Model.destroy(this.model1);
+
+ ns.Model.define('model3');
@edoroshenko
edoroshenko Aug 26, 2013

третья модель не нужна. Используй первую

@edoroshenko edoroshenko commented on an outdated diff Aug 26, 2013
src/ns.model.js
+ throw new Error("[ns.Model] destroyWith'param in " + this.id + " is undefined");
+ }
+
+ if (!Array.isArray(models)) {
+ models = [models];
+ }
+
+ for (var i = 0, len = models.length; i < len; i++) {
+ var model = models[i];
+ if (model instanceof ns.Model) {
+ // при уничтожении модели, с которой связана текущая - она тоже должна быть уничтожена
+ model.on('ns-model-destroyed', function() {
+ ns.Model.destroy(this);
+ }.bind(this));
+ } else {
+ throw new Error("[ns.Model] destroyWith'param in " + this.id + " is not ns.Model");
@edoroshenko
edoroshenko Aug 26, 2013

предлагаю так: "[ns.Model] " + this.id + " destroyWith expected ns.Model or Array of ns.Model in argument"

@edoroshenko edoroshenko commented on an outdated diff Aug 26, 2013
test/spec/ns.model.js
+ this.model1 = ns.Model.get('model1', {id: 1}).setData({key: 1});
+
+ ns.Model.define('model2', {
+ params: {
+ id: null
+ }
+ });
+ this.model2 = ns.Model.get('model2', {id: 1}).setData({key: 1});
+ });
+
+ afterEach(function() {
+ delete this.model1;
+ delete this.model2;
+ });
+
+ it('should not find model after destroy linked model', function() {
@edoroshenko
edoroshenko Aug 26, 2013

should destroy model2 after destroying model1

@doochik doochik commented on an outdated diff Aug 26, 2013
src/ns.model.js
@@ -91,6 +91,32 @@ ns.Model.prototype._bindEvents = function() {
};
/**
+ * Модель должна удалиться вместе с переданными моделями
+ * @param {Model| Array} models - модель или массив моделей
@doochik
doochik Aug 26, 2013

@param { ns.Model | ns.Model[] }

@doochik doochik and 1 other commented on an outdated diff Aug 26, 2013
src/ns.model.js
@@ -91,6 +91,32 @@ ns.Model.prototype._bindEvents = function() {
};
/**
+ * Модель должна удалиться вместе с переданными моделями
+ * @param {Model| Array} models - модель или массив моделей
+ */
+ns.Model.prototype.destroyWith = function(models) {
+ if (!models) {
@doochik
doochik Aug 26, 2013

Мне кажется, это лишняя проверка и пустой массив должен штатно отработать, а то будет неудобно писать

@yanann11
yanann11 Aug 26, 2013

согласна, ведь ниже и так есть проверка - model instanceof ns.Model

@edoroshenko edoroshenko commented on an outdated diff Aug 26, 2013
test/spec/ns.model.js
+ this.model2 = ns.Model.get('model2', {id: 1}).setData({key: 1});
+ });
+
+ afterEach(function() {
+ delete this.model1;
+ delete this.model2;
+ });
+
+ it('should not find model after destroy linked model', function() {
+ this.model2.destroyWith(this.model1);
+ ns.Model.destroy(this.model1);
+
+ expect(ns.Model.find('model2', { id: 1 })).not.to.be.ok();
+ });
+
+ it('should throw on linked model is not nsModel', function() {
@edoroshenko
edoroshenko Aug 26, 2013

should throw error if tried to destroy ns.Model with string

@edoroshenko edoroshenko commented on an outdated diff Aug 26, 2013
test/spec/ns.model.js
+ delete this.model2;
+ });
+
+ it('should not find model after destroy linked model', function() {
+ this.model2.destroyWith(this.model1);
+ ns.Model.destroy(this.model1);
+
+ expect(ns.Model.find('model2', { id: 1 })).not.to.be.ok();
+ });
+
+ it('should throw on linked model is not nsModel', function() {
+ var getmodel = function() { this.model1.destroyWith('string'); };
+ expect(getmodel).to.throwException();
+ });
+
+ it('should throw on linked model is undefined', function() {
@edoroshenko
edoroshenko Aug 26, 2013

should throw error if tried to destroy ns.Model with undefined

@doochik doochik commented on the diff Aug 26, 2013
src/ns.model.js
+ns.Model.prototype.destroyWith = function(models) {
+ if (!models) {
+ throw new Error("[ns.Model] destroyWith'param in " + this.id + " is undefined");
+ }
+
+ if (!Array.isArray(models)) {
+ models = [models];
+ }
+
+ for (var i = 0, len = models.length; i < len; i++) {
+ var model = models[i];
+ if (model instanceof ns.Model) {
+ // при уничтожении модели, с которой связана текущая - она тоже должна быть уничтожена
+ model.on('ns-model-destroyed', function() {
+ ns.Model.destroy(this);
+ }.bind(this));
@doochik
doochik Aug 26, 2013

лишний .bind, событие и так триггерится к контексте модели

@yanann11
yanann11 Aug 26, 2013

в том и дело, что мне нужна не модель, на которой произошло событие, а модель, у которой был вызван метод destroyWith

@doochik
doochik Aug 26, 2013

Понял, сорри :)

@chestozo
chestozo Aug 29, 2013

А мы же не используем .bind() (по стопам @pasaran)?

@doochik
doochik Aug 30, 2013

Для noscript мы это не обсуждали

@yanann11
yanann11 Aug 30, 2013

а для чего не используем?

@edoroshenko edoroshenko commented on an outdated diff Aug 26, 2013
test/spec/ns.model.js
+ });
+
+ afterEach(function() {
+ delete this.model1;
+ delete this.model2;
+ });
+
+ it('should not find model after destroy linked model', function() {
+ this.model2.destroyWith(this.model1);
+ ns.Model.destroy(this.model1);
+
+ expect(ns.Model.find('model2', { id: 1 })).not.to.be.ok();
+ });
+
+ it('should throw on linked model is not nsModel', function() {
+ var getmodel = function() { this.model1.destroyWith('string'); };
@edoroshenko
edoroshenko Aug 26, 2013

Давай перенесём функцию в первый аргумент expect

@yanann11 yanann11 merged commit 1ab054f into master Aug 26, 2013

1 check passed

Details default The Travis CI build passed
@yanann11 yanann11 deleted the destroy-with branch Aug 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment