Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

next: некоторый рефакторинг ns.Model, ns.ModelCollection и совсем немного ns.View #192

Merged
merged 108 commits into from Apr 16, 2014

Conversation

Projects
None yet
4 participants
Member

chestozo commented Jan 18, 2014

Я тут забабахал рефакторинг.
По сути изменений мало, много косметики и дифф, скорее всего, будет совсем страшный.
Мы уже переключились на эту версию, вроде бы, всё хорошо.

Почему я решил это всё замутить?

Главных мотиваций 2:

  1. мне нужно было допилить ns.ModelCollection так, чтобы он не триггерил ns-model-changed в ответ на изменение элемента коллекции.
  2. вот этот код мне не давал покоя: https://github.com/yandex-ui/noscript/blob/master/src/ns.modelCollection.js#L133-L147
    Но это, скорее, в дополнение к первому пункту.

Что сделано?

  1. добавились методы у ns.ModelCollection: onItemChanged, onItemTouched, onItemDestroyed. Так я решил задачу номер раз. Можно переопределить onItemChanged, смотреть что поменялось и дальше триггерить или нет изменение коллекции
    новые методы: https://github.com/yandex-ui/noscript/blob/next/src/ns.modelCollection.js#L119-L136

  2. события элементов коллекции больше не проксируются на коллекцию.
    Зачем это было сделано - затрудняюсь понять.
    На основные события от элементов коллекции (ns-model-changed, ns-model-touched и ns-model-destroyed) - коллекция просто подписывается.
    Часть тестов, которые проверяли, что можно триггерить события на элементе, подписана на них коллекция и коллекция их услышит - удалил.
    было: https://github.com/yandex-ui/noscript/blob/master/src/ns.modelCollection.js#L133-L147
    стало: https://github.com/yandex-ui/noscript/blob/next/src/ns.modelCollection.js#L104-L116

  3. ns.View научилась отписываться от моделей
    было: https://github.com/yandex-ui/noscript/blob/master/src/ns.view.js#L717-L726
    стало: https://github.com/yandex-ui/noscript/blob/next/src/ns.view.js#L290-L300

  4. метод ns.Model.prototype.destroyWith вынес как статический, но сохранил метод в прототипе
    стало так: https://github.com/yandex-ui/noscript/blob/next/src/ns.model.js#L291-L293

  5. Метод ns.Model.isCollection теперь статический (был инстанса).

  6. заменил все попавшиеся на дороге throw new Error(' ... ') на вызов нового метода ns.assert
    сам метод: https://github.com/yandex-ui/noscript/blob/next/src/ns.js#L155-L163
    пример вызова: https://github.com/yandex-ui/noscript/blob/next/src/ns.view.js#L312

  7. 23.03 поддержка фильтров в урлах /{param==filter:type}.

Чего не сделано?

  1. хотел порефакторить ns.ViewCollection, но понял, что мне это прямо сейчас не нужно и решил не трогать.
  2. ns.View просмотрел до половины. Дальше не осилил. Решил тоже забить.

Этот PR он такой: не я уверен, что вы готовы на него переходить.
Но у нас ещё есть несколько серьёзных доработок впереди. singleton view, к примеру, хочется заиметь.
Жду / надеюсь :)

/cc @edoroshenko @doochik

chestozo added some commits Jan 16, 2014

@chestozo chestozo move mc eventlisteners c9a937c
@chestozo chestozo made destroyWith static ed1a3c4
@chestozo chestozo just move static methods to group them all 8441403
@chestozo chestozo more methods sorting 0ea7642
@chestozo chestozo little refactor of the ns.Model set method 82f8dcc
@chestozo chestozo getData little refactor 21262e4
@chestozo chestozo group get data and set data methods 92c3124
@chestozo chestozo little update for setData method 45fa449
@chestozo chestozo no more copypaste in requestParams / key methods d76b69f
@chestozo chestozo make isCollection method static 69bc1f6
@chestozo chestozo easy check ns.Model.isValid af503e0
@chestozo chestozo ns.Model.invalidate little update d01f609
@chestozo chestozo little updates 42ebbe3
@chestozo chestozo little update of ns.Model.isCollection d9ec7d7
@chestozo chestozo comment about ModelUniq 9581b79
@chestozo chestozo rename ns.Model.getKeyParams to ns.Model._getKeyParams 3a81645
@chestozo chestozo started writing simple api docs 2caf8c7
@chestozo chestozo little docs update 4464d51
@chestozo chestozo little note about ns.Model.destroy 14e4699
@chestozo chestozo ns.ModelCollection: refactor subscribe to item events afa29c1
@chestozo chestozo fix jshint and little refactor of ns.ModelCollection._splitModels 8a44064
@chestozo chestozo rename splitUnsubscribe to unsubscribe for shortness and little refac…
…tor in clear
34cfe9e
@chestozo chestozo rename _setData into _beforeSetData 1c20e07
@chestozo chestozo little refactor of _beforeSetData 39562fe
@chestozo chestozo better var name 8f2dcbe
@chestozo chestozo rename info to splitInfo to not confuse it with info f9dadf7
@chestozo chestozo little fixes 39219aa
@chestozo chestozo please, do not write obvious comments 417a288
@chestozo chestozo comments fix 0e62944
@chestozo chestozo collection api doc 4b82bfb
@chestozo chestozo ns.View: just sorting methods 35ee4a4
@chestozo chestozo remove unused _onModelChangeBinded cc9ba15
@chestozo chestozo remove unused method _unbindModels c9a216f
@chestozo chestozo one more little resort 868c9f7
@chestozo chestozo little resort 58b0d53
@chestozo chestozo model _onModelChange was not used 0957520
@chestozo chestozo var names d244387
@chestozo chestozo var names one more time 5543948
@chestozo chestozo move subviews init to separate method f1c3ef4
@chestozo chestozo move big comment out of the method 6ac48e8
@chestozo chestozo rename var for readability 8c0b082
@chestozo chestozo _unbindModels is back + added _bindModel 7a7b820
@chestozo chestozo rename _initInfo into _initInfoEvents b8b0f45
@chestozo chestozo methods resort: move _initInfoParams up b02b08d
@chestozo chestozo ns.assert 1618b28
@chestozo chestozo ns.assert accepts parameters for error message d457f64
@chestozo chestozo use ns.assert part 1 a85189e
@chestozo chestozo more ns.assert usages add9e8d
@chestozo chestozo more ns.assert usage af34328
@chestozo chestozo more comments about view params b3a7f9f
@chestozo chestozo moved create events binding to separate method _bindCreateEvents 8e63238
@chestozo chestozo remove trivial comment 227a472
@chestozo chestozo some comments update 348a099
@chestozo chestozo some more comments 22cc4fa
@chestozo chestozo comment reformat ca7d9ad
@chestozo chestozo comment fix 2263944
@chestozo chestozo one more comment fix a820e8e
@chestozo chestozo fixing tests b7a8e6c
@chestozo chestozo fix collection models init 71a9a92
@chestozo chestozo make a shortcut for ns.Model.destroyWith 1680689
@chestozo chestozo fix renamed internal method _setData 2 _beforeSetData 20aaf9a
@chestozo chestozo typo fixed 0462a29
@chestozo chestozo fix first part of tests: no more triggering collection events via col…
…lection items
0c0f2a2
@chestozo chestozo removed meaningless test e67438d
@chestozo chestozo removed 3 more tests that are meaningless 03cfa8d
@chestozo chestozo fix jscs warning efd3628
@chestozo chestozo ns.model replace all throws to ns.asserts a1b2a6e
@chestozo chestozo added make test b87074b
@chestozo chestozo refactor unsubscribe in modelcollection save way as in view cab4805
@chestozo chestozo collection item events proxy tests part 1 5e2ff7b
@chestozo chestozo test for overriding onItemChanged method 5ffada0
@chestozo chestozo tests for the same model in multiple collections b522f3d
@chestozo chestozo _modelEvents must not be overwritten f685866
Member

chestozo commented Jan 23, 2014

Я в этом PR починил _modelsUnbind у view, но это вышло вот таким боком:
есть view A, она зависит от модели M, всё хорошо
мы её показали A, ушли на другую view B
A отписалась от M

теперь мы меняем что-то в M
Как A должна себя обновить?

Раньше было так, что view от модели не отписывалась. И поэтому любое изменение модели в любой момент было услышано view.
Если же отписывать view от моделей - надо допиливать updateHTML - чтобы при очередном показе view обновилась.

Owner

doochik commented Jan 23, 2014

Можно допилить update, чтобы при обнаружении невалидного view, он делал .invalidate

Member

chestozo commented Jan 23, 2014

view валидно, невалидна модель...

Owner

doochik commented Jan 23, 2014

так значит и view невалидно

Member

chestozo commented Jan 23, 2014

Вот сейчас это не так )
это в новой https://github.com/yandex-ui/noscript/blob/next/src/ns.view.js#L915
это как было (я ничего не менял тут) https://github.com/yandex-ui/noscript/blob/master/src/ns.view.js#L1347

Member

chestozo commented Jan 23, 2014

Поговорили с @doochik и @edoroshenko, попробую резюмировать:
есть 2 подхода (+1 подход, который мне только что пришёл в голову), как view синхронизировать с моделями

  1. подписать view на изменения модели и не отписывать
  2. не подписывать view вообще, а при запуске ns.Update уже инвалидировать нужные части
  3. можно сделать постоянную лёгкую подписку на изменения моделей

1 - это то, как сейчас работает в master
2 - это то, как можно сделать, но не очень нравится @edoroshenko. Мы говорили о том, что плохо на любой change модели перерисовать view. И хоть это решается subview, пока там не всё хорошо.
3 - это фактически lazy вариант 2го пункта: когда мы слышим событие ns-model-changed - сохраняем флаг, что view невалидно.
А потом в методе _tryPushToRequest уже запускаем реальный invalidate.

Надо ещё подумать )

Member

chestozo commented Jan 23, 2014

Итог: пока делаю как Женя сказал. Попутно обнаружил баг - сейчас вью подписывается на изменения моделей при каждом показе. Т.е. в итоге можно на один ns-model-changed словить несколько invalidate-ов ;)

@doochik зацени, вот тут бывает так, что модель в статусе ошибки, а мы ей насильно ставим статус ок.
Вот тесткейс, на мастере повторяется https://gist.github.com/chestozo/8590280

Owner

doochik replied Jan 24, 2014

бажина :(
надо для реквеста тесты оживить )

Member

chestozo commented on 333b6b0 Mar 9, 2014

Лёша, а ты используешь веточку next? )

Member

chestozo commented on c623cba Mar 23, 2014

/cc @doochik @edoroshenko Оказывается тесты неправильно были написаны.
В изначальной конструкции было так:

for (var test in _tests) {
  it(test, function() {
    // test тут был последним значением из цикла for
  });
}

Вот тут появляется возможность инжектить regexp, кстати, что не очень хорошо, но можно считать фичей )

Member

chestozo commented Mar 29, 2014

Member

chestozo commented Mar 29, 2014

Обнаружилась архитектурная особенность:
view сейчас хранит полный набор параметров на момент создания view.
Когда инициализируются модели - используется этот набор параметров.

Возможна такая ситуация:

// 1. params = { context: 'top', login: 'who', id: 1 }
view.key = 'view=image&context=top'
model1.key = 'model=image&id=1'

// 2. params = { context: 'top', login: 'who', id: 2 }
view.key = 'view=image&context=top' // тот же view
model1.key = 'model=image&id=1' // А вот тут - старая модель, хотя должна быть (если исходить из params) такая 'model=image&id=2' 

Поправить это можно только валидацией параметров / ключа модели с учётом параметров / ключа вью.

/cc @doochik тут крутой тест получился - генерация урла должна быть взаимообратной операцией.
У нас уже был decodeURIComponent https://github.com/yandex-ui/noscript/blob/master/src/ns.router.js#L119, а при генерации encodeURIComponent не делался.

@edoroshenko edoroshenko commented on the diff Apr 15, 2014

test/spec/ns.router.js
+ '/{context=:context}/image/{id:int}': 'view'
+ }
+ };
+ ns.router.init();
+ });
+
+ var test_route = function(url, page, params, test_name) {
+ it(test_name || url, function() {
+ expect(ns.router(url)).to.be.eql({ page: page, params: params });
+ });
+ };
+
+ test_route('/search/airport/image/1', 'view', { context: 'search', query: 'airport', id: 1 });
+ test_route('/tag/airport/image/2', 'view', { context: 'tag', tag: 'airport', id: 2 });
+ test_route('/top/image/3', 'view', { context: 'top', id: 3 });
+ });
@edoroshenko

edoroshenko Apr 15, 2014

Contributor

Про эту фичу есть issue #204
Как я там уже отметил, я считаю "фильтр с неявным присваиванием" неудачным решением.

@edoroshenko edoroshenko commented on the diff Apr 15, 2014

src/ns.model.js
}
}
return this;
};
+ns.Model.prototype.needUpdateData = function(data) {
@edoroshenko

edoroshenko Apr 15, 2014

Contributor

Этот метод отвечает на вопрос "устарели ли данные?". Предлагаю назвать его isDataOutdated или isDataObsolete

@chestozo

chestozo Apr 15, 2014

Member

Вопрос "нужно ли обновить данные" модели.
Ну может и надо поменять название, но твои 2 варианта тоже мне не до конца нравятся. Надо ещё подумать.

@edoroshenko

edoroshenko Apr 15, 2014

Contributor

А можешь привести пример конкретной задачи?

@doochik

doochik Apr 15, 2014

Owner

нам эта штука будет полезна для такого кейса:

  • messages сплитися в message
  • один и тот же message можно быть в разных messages
  • соответственно, при сплите нового messages, коллекция обновит данные у message и вызовет перерисовку привязанных view. Хотя на самом деле message почти статичен и у него могут поменяться всего 2 поля
@edoroshenko

edoroshenko Apr 15, 2014

Contributor

не, фича-то нужная. Давайте название правильное придумаем.
Может isDataSame или doesDataEqual ?

@chestozo

chestozo Apr 15, 2014

Member

Посмотрел на backbone - самое близкое, кажется, это hasChanged http://backbonejs.org/#Model-hasChanged
Я бы только назвал hasDataChanged. ok?

@edoroshenko

edoroshenko Apr 15, 2014

Contributor

да, отлично

Contributor

edoroshenko commented Apr 15, 2014

И ещё у меня вопрос. @chestozo , ты говорил, что в next у тебя принципиально другие коллекции. Я этого не заметил. Можешь рассказать, что ты имел в виду?

Member

chestozo commented Apr 15, 2014

Я когда их переделывал, мне казалось, что они сильно отличаться будут, но я старался сохранить дефолтное поведение.
Попробую вспомнить.

@doochik doochik Merge remote-tracking branch 'origin/master' into next
Conflicts:
	README.md
	src/ns.model.js
	src/ns.modelCollection.js
	src/ns.view.js
d6d2e18

@doochik doochik merged commit d6d2e18 into master Apr 16, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@edoroshenko edoroshenko added this to the v0.2.1 milestone Apr 17, 2014

@chestozo chestozo deleted the next branch Feb 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment