Skip to content

feat(api): changeData & update #1834

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

Merged
merged 7 commits into from
Nov 2, 2020
Merged

feat(api): changeData & update #1834

merged 7 commits into from
Nov 2, 2020

Conversation

hustcc
Copy link
Member

@hustcc hustcc commented Oct 31, 2020

@lxfu1

G2Plot v2 暂时没有开放 changeData API,所有的数据更新都是重新走画布渲染,所以无法进行增量数据的更新。但是在 charts 中文档透出,所以导致一些 bug 和答疑问题。

所以需要在 G2Plot 完善 changeData API,并做到文档透出。在开发过程中遇到 update 的另外的问题。分别描述

changeData API

目前的 changeData 仅仅是对 data key 生效,而对于 percent 类型,双轴图的双 data 类型,直方图等经过数据 transform 的均不生效。

解决方案:

  1. 终极方案

实现使用 g2 的 changeData,结果 g2 的 update 能力,可以实现动画,但是需要解决不同图表的 transform 逻辑。目前来看开发成本,回滚成本大。

  1. 中间方案

依然使用 更新整体配置的方式,只不过只更新 data/percent 等数据相关的 key 值。具体伪代码:

 // 针对非 data key 类型的图表,子类实现 changeData 处理数据,调用父类 update 方法, 父类 deepMix 增量数据。
 /**
   * 更新数据
   * @param percent
  */
  public changeData(percent: number) {
    this.update({
      percent,
    });
  }

  // 父类 
  public update(options: any) {
    // this.options 用于处理增量数据,或者所有 getDefaultOptions deepMix base getDefaultOptions()
    this.options = deepMix({}, this.options, this.getDefaultOptions({ ...this.options, ...options }), options);
    this.render();
  }

update API 问题

在实现 changeData 中,发现 update 存在的问题。目前实现代码:

/**
 * 更新配置
 * @param options
 */
public update(options: O) {
  // options 更新是全量更新,这里和构造函数中一会加上图表的默认选项
  this.options = deepMix({}, this.getDefaultOptions(options), options);

  this.render();
}

存在的问题:

  1. 无法增量 update。比如我只 update color 色板,需要外包传入所有配置项;使用上不方便。而 update 不直接开启内置 deepMix 的原因是 配置传入 updefined 的时候,无法覆盖旧的内容。
  2. 如果 getDefaultOptions 中利用 options 生成默认配置的时候,会导致生成的默认配置无法更新

解决办法:

  1. 自己实现 deepMix 逻辑,原 deepMix 逻辑是 undefined 无法做覆盖(Object.assign 是可以覆盖的)

deepMix({},{value: 0}, {value: undefined}) => {value: 0}

因为这个也不算 bug,毕竟 lodash 都是这么做的,所以名字上重命名为: deepAssign 以作区分,方式误导开发者。

  1. deepAssign 针对 !value 类型 直接赋值。
if (Object.prototype.hasOwnProperty.call(sourceObj, key)) {
      const value = sourceObj[key];
      if (!value) {
        target[key] = value;
      } else{
      // TODO 
      }
}

@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2020

😭 Deploy PR Preview e23bac0 failed. Build logs

🤖 By surge-preview

@lgtm-com
Copy link

lgtm-com bot commented Oct 31, 2020

This pull request introduces 3 alerts when merging bfbb9ca into f2bef05 - view on LGTM.com

new alerts:

  • 2 for Prototype pollution in utility function
  • 1 for Unneeded defensive code

@coveralls
Copy link

coveralls commented Oct 31, 2020

Pull Request Test Coverage Report for Build 341531366

  • 130 of 130 (100.0%) changed or added relevant lines in 60 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.08%) to 93.335%

Files with Coverage Reduction New Missed Lines %
src/core/plot.ts 1 97.48%
Totals Coverage Status
Change from base Build 337570953: 0.08%
Covered Lines: 3584
Relevant Lines: 3698

💛 - Coveralls

@hustcc
Copy link
Member Author

hustcc commented Nov 1, 2020

@lxfu1 已经 Review 了一遍,撸了一遍,核心的改动应该有:

  1. plot.update 变成增量,使用更方便,配合解决已有的问题,新增了一个自定义的 deepMix(建议改 deepAssign 便于区分,并完善注释),同时解决现在存在的 update 获取错误的 defaultOptions 问题。
  2. core/plot 的 changeData 利用 增量 update 能力,四个图的 changeData 逻辑单独写
  3. 所有代码的 deepMix 改成新的 deepAssign(是所有的全部都替换吧?)

这个 PR 的 CR 问题(需要解决,具体都有 CR 回复)

  1. deepAssign 属于核心修改,注释完善,写明思考过程,避免后续同学踩坑(一般很难遇到这些问题,无脑用 deepMix 或者 lodash.merge 即可)
  2. deepAssign 写法上应该可以精简,感觉之前 的写法不一定好的
  3. 增量 update 功能,有很多的 deepAssign,但是可能都无法去掉,所以注释需要明确写出这其中的问题。(core 和 adaptor 两个目录的代码,就应该是注释比代码多)
  4. update 中 getDefaultOptions 的参数,是不是也需要 deepAssign?
  5. 新的 update 逻辑需要单独增加单测。针对:
    • getDefaultOptions 使用 options 参数的图表
    • getDefaultOptions 未使用 options 参数的图表

后续可能需要做的:

  1. 如何减少 deep mix?

@lgtm-com
Copy link

lgtm-com bot commented Nov 2, 2020

This pull request introduces 3 alerts and fixes 1 when merging b7e84c4 into f2bef05 - view on LGTM.com

new alerts:

  • 2 for Prototype pollution in utility function
  • 1 for Unneeded defensive code

fixed alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Nov 2, 2020

This pull request introduces 2 alerts and fixes 1 when merging fa6ae31 into f2bef05 - view on LGTM.com

new alerts:

  • 2 for Prototype pollution in utility function

fixed alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Nov 2, 2020

This pull request introduces 2 alerts and fixes 1 when merging 1d0fead into f2bef05 - view on LGTM.com

new alerts:

  • 2 for Prototype pollution in utility function

fixed alerts:

  • 1 for Unused variable, import, function or class

@hustcc
Copy link
Member Author

hustcc commented Nov 2, 2020

先通过了,补充下面几个文件的单测,就合并了, 晚上回归之后,今天发一个版本吧。

image

@lgtm-com
Copy link

lgtm-com bot commented Nov 2, 2020

This pull request introduces 2 alerts and fixes 1 when merging ad13129 into 8d7b4a9 - view on LGTM.com

new alerts:

  • 2 for Prototype pollution in utility function

fixed alerts:

  • 1 for Unused variable, import, function or class

@hustcc hustcc requested a review from lxfu1 November 2, 2020 11:37
@pr-triage pr-triage bot removed the PR: unreviewed label Nov 2, 2020
@hustcc hustcc merged commit e23bac0 into master Nov 2, 2020
@hustcc hustcc deleted the fix/changeData branch November 2, 2020 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants