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

model set时,fire change事件 #17

Closed
errorrik opened this issue Mar 9, 2013 · 42 comments
Closed

model set时,fire change事件 #17

errorrik opened this issue Mar 9, 2013 · 42 comments

Comments

@errorrik
Copy link

errorrik commented Mar 9, 2013

  • name
  • oldValue
  • newValue
@otakustay
Copy link
Member

a1db511 中修复,同时remove方法的调用也会触发相同事件。

@killeryyl
Copy link
Contributor

这会看源码刚看到这里。
联系之前开发里用到的功能,觉得那个silence在这个触发里还是蛮有用的。。。是否可以在触发change的event里再加上silence参数或者在Model里就处理掉?

@otakustay
Copy link
Member

哈哈我昨天刚在和 @errorrikchange事件一出来肯定会有对scilence的要求出现,没想到这么快就出来了。

基本原本的考虑是,这个Model基类提供非常简单的功能,需要额外的(如scilence或者加 getter / setter 之类)功能时,我们可以考虑提供一个patchset,来加强现在的ER。

不论是ViewModelAction,现在的ER提供的是 非常核心 的功能,核心意味着简洁,但在这之上,为了更好地适应业务开发,也肯定有必要提供一个 加强的组件 ,至于要加强到这程度,我现在没想好,所以没有提出来。

因此,关于这个scilence,是放在核心部分,还是未来有一个ObservableModel之类的,希望大家可以讨论一下。

另外, 在event里加上scilence 我不是很理解,我认为如果要加这功能,应该在set的时候传递这么个参数(和Backbone一样),等到事件触发了再处理是来不及的

@killeryyl
Copy link
Contributor

加在event里的初衷是仅修改一点点Model的代码就能把这个silence参数传递给change事件。然后用户在change事件里来处理silence逻辑。不过这肯定不是最好的方法,只是权宜之计。

@otakustay
Copy link
Member

有点奇怪,不是太理解你的说法……

我的理解是,silence这个标记位,用于控制 set属性时是不是触发change事件 ,因此一但事件已经触发,这个值也没有任何意义了,所以要放在set方法里实现。

if (!options.silence) {
    this.fire('change', { newValue ... });
}

放在event里实现是个怎么样的过程,一时想不出来……

@killeryyl
Copy link
Contributor

我的意思大概是这样
Model.prototype.set = function(key, value, options) {
if (arguments.length >= 2) {
var oldValue = this._store[key];
this._store[key] = value;
var event = {
name: key,
oldValue: oldValue,
newValue: value
};
options = options || {}; // 这个可以放到mix里
this.fire('change', reqiure('er/util').mix(options, event));
}
else {
var extension = key;
for (var name in extension) {
this.set(name, extension[name]);
}
}
};
事件event里不仅仅包含事件参数,也包含诸如silence的参数。这样在自定义的changeHandler里就可以根据这个silence来处理了。这样处理可以包含各种各样的事件参数。

@otakustay
Copy link
Member

你这个意思我就明白了,我原来以为你的意思是 把控制权放在event触发时候 ,就感觉似乎搞不定。event触发的时候,传递的相关参数肯定是会一起带过去的,这个放心就好。

至于这个是不是在Model基类中直接实现,我再稍微思考一下。这个涉及到set函数的重载问题,原本就是1-2个参数的重载,加个options后是1-3个参数的重载,逻辑会复杂很多,加之set会是非常非常频繁调用的一个东西,性能不得不考虑

@killeryyl
Copy link
Contributor

嗯,说道性能问题,在Model基类中直接实现silence肯定优于我上面提到的方法。

@otakustay
Copy link
Member

其实我的考虑是,ER框架本身的哲学不提倡做双向绑定,以及Model变化自动更新视图这样的功能。因此,change事件这个功能,并不是ER需要提供的核心功能(通过扩展支持另说),所以不想让这个功能影响到 不用change事件的Model 的性能,因此在这个分支上任何一点变化都会进行细心的衡量:)当然对于功能的支持我是理解的,现在重新打开这个issue,希望有更多人来讨论我们支持到什么程度

@otakustay otakustay reopened this Mar 12, 2013
@otakustay
Copy link
Member

总结一下,现在的set方法重载如下:

{void} set({string} key, {string} value);
{void} set({Object} extension);

如果要实现set时传递一个options参数,重载如下:

{void} set({string} key, {string} value);
{void} set({Object} extension);
{void} set({string} key, {string} value, {Object} options);
{void} set({Object} extension, {Object} options);

从效果上来看是可以完成重载的,但处理重载的逻辑本身就足够影响性能(就set一个值,要先经过N个if确定是哪个重载,结果80%的可能还是项目根本不用change事件)

有一个办法,是再把set拆成2个方法:

{void} set({string} key, {string} value);
{void} set({string} key, {string} value, {Object} options);

{void} mix({Object} extension);
{void} mix({Object} extension, {Object} options);

这样会省力很多,就是判断options有没有提供就行了。

@otakustay
Copy link
Member

@killeryyl 的讨论

嗯,细细想了下,Model.set里的实现,你看这样如何?防止平凡触发change

Model.prototype.set = function(key, value, silence) {
    if (arguments.length >= 2) {
        var oldValue = this._store[key];
        this._store[key] = value;
        var event = {
            name: key,
            oldValue: oldValue,
            newValue: value
        };
        if (silence) {
            return event;
        }
        else {
            this.fire('change', event);
        }
    }
    else if (Object.prototype.toString.call(extension) === '[object Object]' ) {
        var event = {};
        var extension = key;
        for (var name in extension) {
            event[name]=this.set(name, extension[name], silence);
            //或者event是[],这里push进去
        }
        this.fire('change', event);
    }
};

@otakustay
Copy link
Member

arguments.length >= 2的时候,有2种可能:

  1. set(key, value, options)
  2. set(extension, options)

所以 @killeryyl 的代码无法区别这2种重载,具体的4种重载我在上面的讨论中有列出,并不是这段代码的逻辑能够覆盖的。

正确的判断逻辑是,判断key参数的类型(string或object),分为2个分支,但是无论如何判断都导致一个简单的set要有 很多的预分支判断 ,而使用set的人,90%只要赋个值,这里的性能损失不得不考虑(因为这东西用得太频繁)

所以,我还是支持,如果一定要有options这东西的话,把set分为setmix两个方法,以减少内部的判断分支。

@killeryyl
Copy link
Contributor

若接口定义为Array.<Object = {name,oldvalue,newvalue}>
这样无论set(key,value)还是set(object),change里拿到的都是同样结构的数组,仅长度不一,用forEach就能集中操作了。而且也能避免set object时频繁触发change 的问题。在黑匣子外面的理解是无论set什么值,set一次就只触发一次,收到的返回值结构简单或可递归枚举为简单项。

@killeryyl
Copy link
Contributor

function _set(key,value) {
var oldValue = this._store[key];
this._store[key] = value;
var event = {
            name: key,
            oldValue: oldValue,
            newValue: value
        };
}
Model.prototype.set = function(key, value) {
    if (arguments.length >= 2) {
            this.fire('change', [_set.apply(this,arguments)]);
    }
    else if (Object.prototype.toString.call(extension) === '[object Object]' ) {
        var event = [];
        var extension = key;
        for (var name in extension) {
            event.push(_set.call(this, name, extension[name]);
        }
        this.fire('change', event);
    }
};

这个就没有性能损失了,而且还提升了些许性能。

@otakustay
Copy link
Member

这个是MutationObserver的思路,提升change事件接口的复杂性(我相信90%的时间set是只搞定一个值,而不是N个的)来提升性能,可取还很有理由(你看DOM标准就这么搞的)

这个提案用作参考,该Issue继续放置到周末,如果没有什么异议,下周一执行。

因此, @errorrik @DDDBear @Justineo @firede @mytharcher 求讨论,虽然不知道没watch的人会不会通知到……

@mytharcher
Copy link

既然点名了,我就抛块砖。

我之前设计过set+change的双接口的模式,set不触发事件,change触发。看你们之前的讨论,只需要在两个接口判断下不是Object的包装成单值的Object,统一for
in就可以了。当然我是很少考虑性能的,哈哈。

在 2013年3月14日下午2:42,Gray Zhang notifications@github.com写道:

这个是MutationObserver的思路,提升change事件接口的复杂性(我相信90%的时间set
是只搞定一个值,而不是N个的)来提升性能,可取还很有理由(你看DOM标准就这么搞的)

这个提案用作参考,该Issue继续放置到周末,如果没有什么异议,下周一执行。

因此, @errorrik https://github.com/errorrik @DDDBearhttps://github.com/DDDBear
@Justineo https://github.com/Justineo @firedehttps://github.com/firede
@mytharcher https://github.com/mytharcher 求讨论,虽然不知道没watch的人会不会通知到……


Reply to this email directly or view it on GitHubhttps://github.com//issues/17#issuecomment-14888792
.

@otakustay
Copy link
Member

我重新整理一下我的提案。

取消Modelchange事件的支持,提供ObservableModel实现完整的事件机制 。该事件机制如下:

set方法

ObservableModel内部保存一个changeset对象,里面存放了变化的属性列表, set的先后顺序存放

set接口变为以下重载:

  • {void} set({string} name, {*} key, {Object=} options)
  • {void} set({Object} extension, {Object=} options)

set时:

  • 如果options.silencefalsy 的值,则 立即触发change事件
  • 如果options.silence为其它值,则 不触发change事件 ,而是 把该次变化加入到changeset
  • 如果是第2种重载({Object} extension),则在所有属性赋值后,根据options.silence选择是否触发change事件,即一次set 最多只会触发一次change事件

注意上文所说的 触发change事件 指刷新掉所有的变化对象,其过程与下文描述的flush()方法一样。

flush()方法

ObservableModel额外提供一个flush()方法,作用是把changeset中保存的所有变化通过一次change事件发送。

change事件的事件对象如下:

{
    type: 'change',
    changeset: [
        {
            name: 'xxx',
            oldValue: 0,
            newValue: 1,
            type: 'change'
        },
        {
            name: 'yyy',
            oldValue: 0,
            newValue: undefined,
            type: 'remove'
        },
        {
            name: 'zzz',
            oldValue: undefined,
            newValue: 1,
            type: 'add'
        }
    ]
}

该方案参考的是DOM的MutationObserver,把所有变化合成一次提供。其中typeaddremovechange ,根据type不同,oldValuenewValue有不同取值方案,非常直观就不再多解释。

在一次flush()前,如果对同一个属性多次赋值,则只会保留一份变化参数,如假设原来没有xxx,后给了值 abc ,又改为值 xyz ,再flush(),则对应的变化对象如下:

{
    name: 'xxx',
    oldValue: 'abc',
    newValue: 'xyz',
    type: 'change'
}

其中typeadd 的变化对象会消失。

总结

该方案 整合了Backbone和MutationObserver 的设计思路,整合 @killeryyl合并变化对象 以及 @mytharcher设置和事件分开 的想法,同时因为对set方法有较大的逻辑影响,因此考虑不放在Model基类中,保持Model本身的纯净,以额外的对象提供完善而全面的高级功能。

@otakustay
Copy link
Member

补充一些误区:

第一,这属性叫silent,不叫silence,词性搞错了……

silent这个应该控制 此次变化不记入change事件中 ,即完全不进到对象的changeset,无论何时调用flush(),都不会出现这次set带来的变化。如果silent设置的属性依旧会在change事件中出现,那么就没有任何办法解决死循环式的 set -> change -> set 这种问题。

因此,对于要 延迟触发change事件的场景 ,应该添加一个叫lazy的属性,默认为 false ,为 true 时当次set提供的属性变化进入changeset中,等下一次flush()时再提供。

由于这个Issue有了更新,因此讨论截止推迟至下周三18:00,如果没有什么异议,计划是按这个方案执行。

@errorrik
Copy link
Author

我的建议还是直接在Model里支持事件,这是个基本性能无损的事情。

然后不太明白的是,为啥set有集合设置功能

@errorrik
Copy link
Author

没有任何办法解决死循环式的 set -> change -> set 这种问题。

这种不用解决吧,如果写成死循环我认为活该。。。

@otakustay
Copy link
Member

原定的set函数就支持传一个对象进来,形成类似mix的效果的,这个要去掉吗,我认为可以改个名,但去掉不合适(因为Model里本来就有逻辑依赖这个)。

在Model中支持的话,几个问题:

  • 每次属性变化都触发事件,还是收集起来等flush?
  • 如果不是每次立即触发的话,要不要有参数支持延迟等flush触发?
  • 是不是需要提供change:xyz这种的单属性变化的事件以方便监听?
  • 死循环不处理的话,silent有多少意义?

@errorrik
Copy link
Author

延迟触发change事件的场景

举个栗子?

@otakustay
Copy link
Member

就是上面讨论的,通过lazy标识来延迟触发,等主动调用flush()flush()后会给个数组里面有所有的变化信息。这个其实有点像我们ESUI中控件的setProperties方法,多次变化收集起来去刷一次界面。

@errorrik
Copy link
Author

原定的set函数就支持传一个对象进来,形成类似mix的效果的,这个要去掉吗

以前是支持的?我个人觉得这个功能可以有,但是:

  1. 对使用者而言,是批量设置,mix只是er开发者的实现细节,不适用于api命名
  2. 最好是一个新的批量设置接口

@otakustay
Copy link
Member

可以把批量设置抽出来,接口名如果mixmergeextend都不合适的话,就叫setBatch好了……

同时一个问题,批量设置的时候,change触发多次还是一次(这个和前面的延迟触发一个话题)

@errorrik
Copy link
Author

每次属性变化都触发事件,还是收集起来等flush?
如果不是每次立即触发的话,要不要有参数支持延迟等flush触发?

我认为,每次调用应该都是立即触发的。有几个人真正知道io的flush是干嘛的……

是不是需要提供change:xyz这种的单属性变化的事件以方便监听?

从易用性上来看有必要,但是使用者其实可以搞个代理。我觉得不提供这货不是硬伤。而且,on('change:xyz'我觉得很长时间内都不会被广泛认知

死循环不处理的话,silent有多少意义?

silent的意义还在于,如果你提供了视图自动更新,它可以有一种途径让视图别动

@errorrik
Copy link
Author

批量设置的时候,change触发多次还是一次(这个和前面的延迟触发一个话题)

我觉得是触发一次。任何一次方法调用,触发一次。

@errorrik
Copy link
Author

接口名如果mix、merge、extend都不合适的话,就叫setBatch好了

命名是永远的痛

@otakustay
Copy link
Member

silent的意义还在于,如果你提供了视图自动更新,它可以有一种途径让视图别动

silent的问题在于,一但设置了,就再也不会触发了(原来的lazy还是有机会触发的),那么如果要设置一堆再触发,就要set几次,然后 手动 把Model再丢给视图渲染一次,不能通过事件处理函数进行。为此视图要给2个接口,一个对应change事件的处理函数接受一个Event对象,一个接受Model对象处理全部属性(还不知道哪些变过)

on('change:xyz')我觉得很长时间内都不会被广泛认知

这东西是backbone搞出来的,不是我的创意……

我觉得是触发一次

触发一次的话,这一次里肯定有一个数组,提示所有的变化信息。那么单个的set,触发的时候,事件对象里的是数组还是单纯一个对象?这里的一致性如何保证?

@errorrik
Copy link
Author

接口名如果mix、merge、extend都不合适的话,就叫setBatch好了

setBatch感觉是设置batch这个东西。不如叫batchSet...我鸟纹不好,就一提

@errorrik
Copy link
Author

那么如果要设置一堆再触发,就要set几次,然后 手动 把Model再丢给视图渲染一次,不能通过事件处理函数进行

走批量设置接口

这东西是backbone搞出来的,不是我的创意……

我没调研过,关键是多少人会这么用。

触发一次的话,这一次里肯定有一个数组,提示所有的变化信息。那么单个的set,触发的时候,事件对象里的是数组还是单纯一个对象?这里的一致性如何保证?

我临时有两种提案

  1. 统一使用eventArgument.changes。changes的key是data name,value是{newValue, oldValue}
  2. eventArgument.changeType="single|batch",single时直接有newValue, oldValue,batch时同上。

@otakustay
Copy link
Member

eventArgument.changes这个方案我觉得意义不大,注册change事件正因为 不知道哪个属性会变 ,你还怎么让他用key来从changes里拿到东西,铁定会用for .. in来跑,那么还不如是个数组。

changeType这个,反正batch时还是同上,感觉直接用一个方案更好,比较一下(我先假设changes是数组,因为实在想不到object有啥用):

changeType时:

if (changeType === 'single') {
    renderProperty(e.name, e.newValue);
}
else {
    e.changes.forEach(function(c) { renderProperty(c.name, c.newValue); });
}

changeType时就是统一的:

e.changes.forEach(function(c) { renderProperty(c.name, c.newValue); });

只省不多……

@errorrik
Copy link
Author

注册change事件正因为 不知道哪个属性会变 ,你还怎么让他用key来从changes里拿到东西,铁定会用for .. in来跑

用kv的原因是,如果用户有明确的倾向,他就可以if (name in changes)。我把思考过程抛出来,仅供参考

@errorrik
Copy link
Author

changeType这个,反正batch时还是同上,感觉直接用一个方案更好,比较一下(我先假设changes是数组,因为实在想不到object有啥用):

嗯,确实是统一的更好

@otakustay
Copy link
Member

用kv的原因是,如果用户有明确的倾向,他就可以if (name in changes)

不知道现在各业务线对这个东西的使用是怎么样的?如果是标准的MVVM模式的话,没有这样的需求。如果是非常明确的业务逻辑的话,似乎是有这样的需要的。

我对于用object的担心是for.. in控制不好就会产生prototype上属性的问题,以及没办法用原生的forEach,如果各团队能严格控制住的话没问题

@errorrik
Copy link
Author

如果真的碰到有Object.prototype.xxx的,我觉得er可以不拒绝不负责。

其实都可以,什么数据结构都有利弊。也确实是遍历的需求多的多。那就List吧

@otakustay
Copy link
Member

好,总结下:

  • change事件的Event对象里有changes数组,数组中每一项是一个变化记录
  • 每一个变化记录包含nameoldValuenewValuetype共4个属性,其中typeadd / remove / change 之一,不知道有没有用,但反正没什么代价,提供一下
  • 一次setbatchSet触发一次change事件
  • 可以提供silent参数,这样就不触发change事件了

batchSet命名再稍微纠结下,setMultiple是不是也不合适?

@errorrik
Copy link
Author

batchSet命名再稍微纠结下,setMultiple是不是也不合适?

我坐地铁去,回家路上再想想。。。

@killeryyl
Copy link
Contributor

提醒下,别忘了可以这样(之前就提过的说做参考,现在优化了点,再看看吧)
function _set(key,value) {
var oldValue = this._store[key];
var type = 'change';
if (oldVlaue === null || oldVlaue === undefinded) type = 'add';
if (value=== null || value=== undefinded) type = 'remove';
this._store[key] = value;
var event = {
type: type,
name: key,
oldValue: oldValue,
newValue: value
};
}
Model.prototype.set = function(key, value, slience) {
var event = [];
if (Object.prototype.toString.call(extension) === '[object Object]' ) {
slience = value
var extension = key;
for (var name in extension) {
event.push(_set.call(this, name, extension[name]);
}
}
else if (key) {
event.push(_set.call(this, key, value));
}
if(!silence && event.length>0) this.fire('change', event);
};
把set里的核心功能提出来,这样就不用纠结反复调用出的问题了。
常理上来说用户使用一次set只触发一次change,用数组存取事件参数也是为了set单值和对象时都能保证对外输出数据的一致性。

@errorrik
Copy link
Author

命名的问题,昨天没想到啥好的,今天起床突然有一个词fill

@otakustay
Copy link
Member

@killeryyl 最新的方案也有参考这个方法,但 @errorrik 提出的是set从语义上不适合提供 多属性赋值 这样的行为,因此分为2个名字更合适。如果分为2个接口,那么原来的set本身就是类似这里的_set的效果,可重用

@otakustay
Copy link
Member

7fa90a3 中重新实现了一系列的方法,参考了 @killeryyl@errorrik 的思想:

  • 批量设置的接口命名为fill
  • 抽取setProperty辅助函数,setfill统一使用该函数
  • { silent: true }来禁止触发change事件
  • load()过程中所有的赋值均不会触发change事件

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

No branches or pull requests

4 participants