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

refactor: 代码优化 & 补全 e2e 测试 #161

Merged
merged 14 commits into from
Feb 17, 2020

Conversation

donaldshen
Copy link
Collaborator

@donaldshen donaldshen commented Feb 14, 2020

Why

优化一波代码,为后续新功能铺垫。

How

大致按 commit 分。除了 format 相关的改动有后期调整

移除 package-option.js

  • 原本用 h 函数渲染的 select 和 radio 组件等的选项,现在用 template 实现。
  • 基本上完全移除了 h 函数现在。
  • mixin -1

将方法 getEnableWhenSatatus 改造成纯函数

  • mixin -1

移除 mixin-hidden

  • 将方法 getHiddenStatus 改造成计算属性 hiddenStatus
  • 将方法 getEnableWhenStatus 改造成计算属性 enableWhenStatus
  • 现在有个行为变更:原本有 hidden 会完全忽略 enableWhen 属性;现在是否显示取决于 hidden & enableWhen 的并集
  • mixin -1 = 0

调整层级

  • vue 组件移到 components 目录下
  • js 文件移到 util 目录下

优化 transformContent

通过函数名称明确 transformContent 的行为:

  • $前缀的 key 去掉该前缀
  • 抽取 component 中的 rules

优化初始化 options & value 的函数

这两个函数的本质是,从 content 中抽取 options & default 值,并转成树🌲的形式。
这里我抽离出了共同的 collect 逻辑

明确 updateForm 的合并逻辑(结合 commit: 补回 inputFormat 功能)

这是原本的逻辑,bug 我注释其中

/**
 * values: 传入的新值
 */
const updateValue = content => {
  return content.reduce((acc, item) => {
    let value;
    if (item.type === GROUP) {
      value = updateValue(item.items); // BUG: 递归到下一层后,values 应该也要进入下一层
    } else {
      value =
        typeof item.inputFormat === "function"
          ? item.inputFormat(values)
          : values[item.id]; // 这里可以看到传入的 key 不能是路径
    }
    if (value !== undefined) {
      _set(acc, item.id, value); // BUG: 如果传入的 key 是路径,那这里不该用 item.id,整个递归逻辑也得重写
    }
    return acc;
  }, {});
};

新的递归合并 oldV & newV 策略如下:

  1. 根据 inputFormat 对 newV 做一遍处理
  2. 过滤掉 newV 中不存在于 content 中的项
  3. 如果该项的 type 不是 GROUP,直接覆盖合并到 oldV
  4. 如果是,oldV & newV & content 一起进入下一层,递归执行步骤 2 到 4

getFormValue(结合 commit: outputFormat 返回值是 Object 类型时要覆盖到上一层)

功能没有改动,补充了对关键转换函数的测试

删除 updateOptions

这个函数是 2018 年添加的,其监听 innerContent 的变化,然后更新内部 options。
问题是,现在的 innerContent 是一次性的,beforeMount 时生成。更新 options 已经有 setOptions 来实现了,没人直接改 content。
删了后,测试 basic, set-options, remote 几个示例正常

补回 inputFormat 功能

之前重构 updateForm 时漏掉了。这里补回并增加单测

补全 e2e 测试

终于,把关键用例的 e2e 测试补全了。不仅发现了很多之前 commit 的遗漏,还能让以后的重构更安心

Test

单元测试

通过,显然,否则不能 push

e2e 测试

image

@auto-add-label auto-add-label bot added the WIP label Feb 14, 2020
@netlify
Copy link

netlify bot commented Feb 14, 2020

Deploy preview for el-form-renderer ready!

Built with commit 3acdcf1

https://deploy-preview-161--el-form-renderer.netlify.com

@donaldshen donaldshen changed the title WIP: Refactor refactor: 代码优化 Feb 14, 2020
@auto-add-label auto-add-label bot added refactor and removed WIP labels Feb 14, 2020
import getEnableWhenSatatus from './mixins/enable-when'
import {noop} from './utils'
import getEnableWhenSatatus from '../util/enable-when'
import {noop} from '../util/utils'
Copy link
Member

Choose a reason for hiding this comment

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

写在 util/index.js,引入 ../util 就好了?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

暂时先这样吧:
util/utils - 通用函数
util/enable-when - getEnableWhenSatatus 的逻辑
util/transform-content - 转换 content 供内部使用的函数,这个在重构中,会更明确其意义些

src/util/enable-when.js Outdated Show resolved Hide resolved
@donaldshen donaldshen changed the title refactor: 代码优化 test: 代码优化 & 补全 e2e 测试 Feb 17, 2020
@auto-add-label auto-add-label bot added test and removed refactor labels Feb 17, 2020
@donaldshen donaldshen changed the title test: 代码优化 & 补全 e2e 测试 refactor: 代码优化 & 补全 e2e 测试 Feb 17, 2020
@auto-add-label auto-add-label bot added refactor and removed test labels Feb 17, 2020
@donaldshen donaldshen merged commit 1852bf8 into FEMessage:dev Feb 17, 2020
@donaldshen donaldshen deleted the refactor branch February 17, 2020 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants