Skip to content

Conversation

@L-fingerprint
Copy link
Contributor

refs NEXT_BUILDER-1908

依赖检查

组件之间的依赖声明,是微服务组件架构下的重要信息,请确保其正确性。

请勾选以下两组选项其中之一:

  • 本次 MR 没有使用上游组件(例如框架、后台组件等)的较新版本提供的特性。

或者:

  • 本次 MR 使用了上游组件(例如框架、后台组件等)的较新版本提供的特性。
  • 在对应的文件中更新了该上游组件的依赖版本(或确认了当前声明的依赖版本已包含本次 MR 使用的新特性)。

提交信息检查

Git 提交信息将决定包的版本发布及自动生成的 CHANGELOG,请检查工作内容与提交信息是否相符,并在以下每组选项中都依次确认。

破坏性变更是针对于下游使用者而言,可以通过本次改动对下游使用者的影响来识别变更类型:

  • 下游使用者不做任何改动,仍可以正常工作时,那么它属于普通变更。
  • 反之,下游使用者不做改动就无法正常工作时,那么它属于破坏性变更。

例如,构件修改了一个属性名,小产品 Storyboard 中需要使用新属性名才能工作,那么它就是破坏性变更。
又例如,构件还没有任何下游使用者,那么它的任何变更都是普通变更。

破坏性变更:

  • ⚠️ 本次 MR 包含破坏性变更的提交,请继续确认以下所有选项:
  • 没有更好的兼容方案,必须做破坏性变更。
  • 使用了 feat 作为提交类型。
  • 标注了 BREAKING CHANGE: 你的变更说明
  • 同时更新了本仓库中所有下游使用者的调用。
  • 同时更新了本仓库中所有下游使用者对该子包的依赖为即将发布的 major 版本。
  • 同时为其它仓库的 Migrating 做好了准备,例如文档或批量改动的方法。
  • 手动验证过破坏性变更在 Migrate 后可以正常工作。
  • 破坏性变更所在的提交没有意外携带其它子包的改动。

新特性:

  • 本次 MR 包含新特性的提交,且该提交不带有破坏性变更,并使用了 feat 作为提交类型。
  • 给新特性添加了单元测试。
  • 手动验证过新特性可以正常工作。

问题修复:

  • 本次 MR 包含问题修复的提交,且该提交不带有新特性或破坏性变更,并使用了 fix 作为提交类型。
  • 给问题修复添加了单元测试。
  • 手动验证过问题修复得到解决。

杂项工作:

即所有对下游使用者无任何影响、且没有必要显示在 CHANGELOG 中的改动,例如修改注释、测试用例、开发文档等:

  • 本次 MR 包含杂项工作的提交,且该提交不带有问题修复、新特性或破坏性变更,并使用了 chore, docs, test 等作为提交类型。

@cypress
Copy link

cypress bot commented Aug 22, 2022



Test summary

12 0 0 0


Run details

Project next-core
Status Passed
Commit 9d4ee0b ℹ️
Started Aug 22, 2022 1:20 PM
Ended Aug 22, 2022 1:21 PM
Duration 01:08 💡
OS Linux Ubuntu - 20.04
Browser Electron 100

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@coveralls
Copy link

coveralls commented Aug 22, 2022

Pull Request Test Coverage Report for Build 2904128008

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 95.463%

Totals Coverage Status
Change from base Build 2902122022: 0.02%
Covered Lines: 7440
Relevant Lines: 7692

💛 - Coveralls

},
{
method: "post",
url: ".*cmdb.instance.PostSearch.*",
Copy link
Member

Choose a reason for hiding this comment

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

这个是什么规则?.* 是正则吗?那中间其他的 . 呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是的,这边是希望是url能够支持正则匹配的,这个中间的,我这边我应该加个\,去匹配正确的.

Copy link
Member

Choose a reason for hiding this comment

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

规则在哪定义的?能否直接传正则类型过来?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

但是因为我这边用法是直接用new RegExg() 去做的,这样也是能匹配正确的,这边规则是在visual builder的config那里去定义的,可以直接传正则类型过来,就是为了避免白名单重复相似接口

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

这里的规则是:

- method: GET
  uri: .+
- method: POST
  uri: ^.+cmdb\.instance\.PostSearch\/.+$

准确来说,这里的配置意思是:主动清除缓存的请求白名单。
意思是,命中到这里面名单的请求都不会触发清除缓存动作,否则就触发清除缓存动作(比如PUT/DELETE等请求)

Copy link
Contributor

Choose a reason for hiding this comment

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

目前来看,只需要满足method和uri的匹配规则就够了,至于请求body或返回body的匹配,暂时不需要,以后有需要可以再扩展

Copy link
Member

Choose a reason for hiding this comment

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

有点拗口🤦‍♂️,我看错代码实现了。不过没有理解为啥需要「清除缓存」,匹配后不去记录缓存,不就行了?

Copy link
Member

Choose a reason for hiding this comment

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

说错了,应该是只有匹配的记录才进行缓存,不需要有「清除缓存」的动作

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不是哦,这边是匹配到的接口,应该是后续再去触发的时候,不用缓存那里的数据,而是直接发送请求,因为我们首次进去这边都会缓存一次(原来做法)因为首次都是需要拉取接口的,我这边实现就觉得没必要在第一次就去判断哪些接口需要不进入「缓存列表」,而是在再次触发的时候,清除对应在 requestCacheIgnoreList 的缓存

Comment on lines 349 to 350
this.isEnableCache &&
!this.requestCacheWhiteListKeys.has(key) &&
this.requestCache.set(key, promise);
Copy link
Member

Choose a reason for hiding this comment

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

如果这个列表的作用是「忽略预览缓存」,那不应该叫「白名单」,白名单是「允许名单内的东西做什么」的意思,所以这里严格上应该叫「黑名单」,不过叫「requestCacheIgnoreList」更容易理解。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,那我这边改一下字段

Copy link
Contributor

Choose a reason for hiding this comment

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

requestCacheIgnoreList赞同这个

);
if (isExistRequestCacheWhite) {
this.requestCacheWhiteListKeys.add(key);
this.requestCache.delete(key);
Copy link
Member

Choose a reason for hiding this comment

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

看了下对应的卡片,理解需求了。astrid 这里似乎并没有实现「清理缓存」的功能,应该这样?

Suggested change
this.requestCache.delete(key);
this.requestCache.clear();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不是全清哦,只是对应匹配到的key清除哦

Copy link
Member

Choose a reason for hiding this comment

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

@Alren-huang 我看需求是说的「清理所有当前缓存」吧?

@L-fingerprint L-fingerprint force-pushed the astrid/cacheWhiteList branch from 774e560 to 1b806d7 Compare August 22, 2022 06:26
@L-fingerprint L-fingerprint changed the title feat(): support previewRequestCacheWhiteList feat(): support requestCacheIgnoreList Aug 22, 2022
@L-fingerprint L-fingerprint force-pushed the astrid/cacheWhiteList branch 2 times, most recently from 354f922 to 5c00cf1 Compare August 22, 2022 10:41
public setClearPreviewRequestCacheIgnoreList = (
list: ClearPreviewRequestCacheIgnoreListConfig[]
): void => {
this.clearPreviewRequestCacheIgnoreList = list;
Copy link
Contributor

Choose a reason for hiding this comment

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

setRequestCacheList & requestCacheList
这个命名应该就OK了
setClearPreviewRequestCacheIgnoreList太拗口了

req.method === config.method &&
(!req.uri || new RegExp(req.uri).test(config.url))
);
if (this.isRender || isExistRequestCache) {
Copy link
Contributor

Choose a reason for hiding this comment

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

这里需要添加多一点注释

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 347 to 353
const isExistRequestCache = this.clearPreviewRequestCacheIgnoreList.some(
(req) =>
req.method === config.method &&
(!req.uri || new RegExp(req.uri).test(config.url))
);
if (this.isRender || isExistRequestCache) {
this.requestCache.set(key, promise);
}
if (!this.isRender && !isExistRequestCache) {
this.requestCache.clear();
}
Copy link
Member

@weareoutman weareoutman Aug 22, 2022

Choose a reason for hiding this comment

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

Suggested change
const isExistRequestCache = this.clearPreviewRequestCacheIgnoreList.some(
(req) =>
req.method === config.method &&
(!req.uri || new RegExp(req.uri).test(config.url))
);
if (this.isRender || isExistRequestCache) {
this.requestCache.set(key, promise);
}
if (!this.isRender && !isExistRequestCache) {
this.requestCache.clear();
}
const shouldClearCache = !this.clearCacheIgnoreList.some(
(req) =>
req.method === config.method &&
(!req.uri || new RegExp(req.uri).test(config.url))
);
if (shouldClearCache) {
this.requestCache.clear();
} else {
this.requestCache.set(key, promise);
}

@L-fingerprint L-fingerprint force-pushed the astrid/cacheWhiteList branch 3 times, most recently from 673c1cf to b9d4eb7 Compare August 22, 2022 13:12
@L-fingerprint L-fingerprint force-pushed the astrid/cacheWhiteList branch from b9d4eb7 to f7a6d41 Compare August 22, 2022 13:13
@weareoutman weareoutman merged commit 485b81e into master Aug 22, 2022
@weareoutman weareoutman deleted the astrid/cacheWhiteList branch August 22, 2022 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants