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

Deps lint #15354

Merged
merged 18 commits into from Mar 14, 2019
Merged

Deps lint #15354

merged 18 commits into from Mar 14, 2019

Conversation

zombieJ
Copy link
Member

@zombieJ zombieJ commented Mar 12, 2019

First of all, thank you for your contribution! 😄

New feature please send pull request to feature branch, and rest to master branch.
Pull request will be merged after one of collaborators approve.
Please makes sure that these form are filled before submitting your pull request, thank you!

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / document update
  • Component style update
  • TypeScript definition update
  • Refactoring
  • Code style optimization
  • Branch merge
  • Other (about what?)

👻 What's the background?

ref: ant-design/antd-tools#92

Kapture 2019-03-12 at 22 04 21

close #15407

💡 Solution

antd-tools add style deps check

☑️ Self Check before Merge

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

@netlify
Copy link

netlify bot commented Mar 12, 2019

Deploy preview for ant-design ready!

Built with commit f9d8e50

https://deploy-preview-15354--ant-design.netlify.com

@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #15354 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15354      +/-   ##
==========================================
- Coverage   94.11%   94.04%   -0.07%     
==========================================
  Files         250      250              
  Lines        6643     6653      +10     
  Branches     1927     1949      +22     
==========================================
+ Hits         6252     6257       +5     
- Misses        390      395       +5     
  Partials        1        1
Impacted Files Coverage Δ
components/page-header/index.tsx 100% <ø> (ø) ⬆️
components/transfer/index.tsx 85.43% <0%> (-6.43%) ⬇️
components/badge/ScrollNumber.tsx 96.92% <0%> (-0.14%) ⬇️
components/form/FormItem.tsx 96.51% <0%> (-0.12%) ⬇️
components/form/context.ts 100% <0%> (ø) ⬆️
components/form/Form.tsx 88.88% <0%> (ø) ⬆️
components/list/index.tsx 97.26% <0%> (ø) ⬆️
components/breadcrumb/BreadcrumbItem.tsx 100% <0%> (ø) ⬆️
components/menu/SubMenu.tsx 75% <0%> (ø) ⬆️
components/list/Item.tsx 100% <0%> (ø) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97880a9...8eef9fb. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #15354 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15354      +/-   ##
==========================================
- Coverage   94.11%   94.04%   -0.07%     
==========================================
  Files         250      250              
  Lines        6643     6653      +10     
  Branches     1927     1949      +22     
==========================================
+ Hits         6252     6257       +5     
- Misses        390      395       +5     
  Partials        1        1
Impacted Files Coverage Δ
components/page-header/index.tsx 100% <ø> (ø) ⬆️
components/transfer/index.tsx 85.43% <0%> (-6.43%) ⬇️
components/badge/ScrollNumber.tsx 96.92% <0%> (-0.14%) ⬇️
components/form/FormItem.tsx 96.51% <0%> (-0.12%) ⬇️
components/form/context.ts 100% <0%> (ø) ⬆️
components/form/Form.tsx 88.88% <0%> (ø) ⬆️
components/list/index.tsx 97.26% <0%> (ø) ⬆️
components/breadcrumb/BreadcrumbItem.tsx 100% <0%> (ø) ⬆️
components/menu/SubMenu.tsx 75% <0%> (ø) ⬆️
components/list/Item.tsx 100% <0%> (ø) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97880a9...f9d8e50. Read the comment docs.

@@ -1,5 +1,6 @@
import '../../style/index.less';

// style dependencies
// deps-lint-skip: tooltip, popover
Copy link
Member

@afc163 afc163 Mar 12, 2019

Choose a reason for hiding this comment

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

这是啥?感觉各组件会很难判断要怎么写。

Copy link
Member Author

Choose a reason for hiding this comment

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

popconfirm:

  • 用了 tooltip 组件但是没有用它的样式
  • 用了 popover 的样式但是没用它的组件

前者目前只能加到 skip 里(但是这种场景很少见)。后者主要是组件抽象问题,比如:

  1. 为了复用样式设置了相同的 prefix。
  2. 没有暴露出诸如 renderInput 这种方法,因而直接设置了 ant-input 样式进去。

后者是遗留问题,现在新设计的组件都会暴露出来或者直接复用组件,不会有这个问题。

总结下来,就是新的组件应该都用不到 deps-lint-skip

Copy link
Member Author

Choose a reason for hiding this comment

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

注:如果我们重构这些用了 deps-lint-skip 的组件,重构后的代码也应该是没有 deps-lint-skip的才对。

@zombieJ zombieJ requested review from chenshuai2144 and afc163 and removed request for chenshuai2144 March 12, 2019 15:14
@@ -1,2 +1,5 @@
import '../../style/index.less';

// style dependencies
// deps-lint-skip: grid
import '../../grid/style/index.less';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import '../../grid/style/index.less';
import '../../grid/style';

@@ -1,4 +1,6 @@
import '../../style/index.less';
import './index.less';

// style dependencies
// deps-lint-skip: input
Copy link
Member

Choose a reason for hiding this comment

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

这是不是有问题,其实是应该引入 Input 样式的,可以建一个 demo 项目 import 试一下。

下面的都可以排查一下。

Copy link
Member Author

Choose a reason for hiding this comment

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

代码里依赖了 Select 和 Input,而依赖的 select 里,引入了 input 样式。所以这边 input 可加可不加,但是 lint 检测出来直接调用了 Input 就告知少了 input 的样式。

Copy link
Member Author

Choose a reason for hiding this comment

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

这边 input 也改成直接 import style 好了,以后万一 select 没了 input,样式就挂了(虽然不可能的事情)。

Copy link
Member Author

Choose a reason for hiding this comment

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

看了一下,因为 auto-complete 是从 select 里抽出来的。以前的 input style 依赖在 select 里已经没用了。所以应该是:

select:
- input

auto-complete:
+ input

Copy link
Member

Choose a reason for hiding this comment

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

对,所以这个 lint 是起作用了,不然都没发现。

Copy link
Member

Choose a reason for hiding this comment

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

其他也可以排查一下。

@@ -2,4 +2,5 @@ import '../../style/index.less';
import './index.less';

// style dependencies
// deps-lint-skip: row, col
Copy link
Member Author

Choose a reason for hiding this comment

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

@ztplz
Copy link
Contributor

ztplz commented Mar 13, 2019

这个是干啥的 检查依赖的吗

@zombieJ
Copy link
Member Author

zombieJ commented Mar 13, 2019

这个是干啥的 检查依赖的吗

antd 里支持单独 import component:

import Button from 'antd/lib/button';
import 'antd/lib/button/style/css.js';

有的组件会依赖其他组件,所以需要保证组件的 style 里将依赖的组件 style include 进来。
这个就是用来检查 style 依赖的。

@zombieJ zombieJ mentioned this pull request Mar 14, 2019
1 task
@zombieJ
Copy link
Member Author

zombieJ commented Mar 14, 2019

用了 style 正好被 lint 抓出来了:#15407

感觉还可以改进一下,禁止直接 import { xxx } from '../index';

@afc163
Copy link
Member

afc163 commented Mar 14, 2019

其实 style/index.tsx 文件最好都不用提交,发布的时候自动生成。

@zombieJ
Copy link
Member Author

zombieJ commented Mar 14, 2019

其实 style/index.tsx 文件最好都不用提交,发布的时候自动生成。

有些只用组件不用样式的自动生成就不太好判断了,搞成如果没有的话 lint 跑了就自动生成。如果有了就做检查会比较好点。

@afc163
Copy link
Member

afc163 commented Mar 14, 2019

有些只用组件不用样式的自动生成就不太好判断了

这种也不用判断,很难保证以后重构会不会用到样式。

@zombieJ
Copy link
Member Author

zombieJ commented Mar 14, 2019

有些只用组件不用样式的自动生成就不太好判断了

这种也不用判断,很难保证以后重构会不会用到样式。

意思是不用也引进来?

@afc163 afc163 merged commit c1ef989 into master Mar 14, 2019
@delete-merged-branch delete-merged-branch bot deleted the deps-lint branch March 14, 2019 12:34
@afc163
Copy link
Member

afc163 commented Mar 14, 2019

对,其实你很难判断用没用到。

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.

Page Header Disables Tree Shaking
3 participants