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

Conversation

Projects
None yet
3 participants
@zombieJ
Copy link
Member

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

zombieJ added some commits Mar 8, 2019

@netlify

This comment has been minimized.

Copy link

commented Mar 12, 2019

Deploy preview for ant-design ready!

Built with commit f9d8e50

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

@codecov

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
@afc163

afc163 Mar 12, 2019

Member

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

This comment has been minimized.

Copy link
@zombieJ

zombieJ Mar 12, 2019

Author Member

popconfirm:

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

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

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

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

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

This comment has been minimized.

Copy link
@zombieJ

zombieJ Mar 12, 2019

Author Member

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

@zombieJ zombieJ requested review from chenshuai2144 and afc163 and removed request for chenshuai2144 Mar 12, 2019

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

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

This comment has been minimized.

Copy link
@afc163

afc163 Mar 13, 2019

Member
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

This comment has been minimized.

Copy link
@afc163

afc163 Mar 13, 2019

Member

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

下面的都可以排查一下。

This comment has been minimized.

Copy link
@zombieJ

zombieJ Mar 13, 2019

Author Member

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

This comment has been minimized.

Copy link
@zombieJ

zombieJ Mar 13, 2019

Author Member

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

This comment has been minimized.

Copy link
@zombieJ

zombieJ Mar 13, 2019

Author Member

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

select:
- input

auto-complete:
+ input

This comment has been minimized.

Copy link
@afc163

afc163 Mar 13, 2019

Member

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

This comment has been minimized.

Copy link
@afc163

afc163 Mar 13, 2019

Member

其他也可以排查一下。

zombieJ added some commits Mar 13, 2019

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

// style dependencies
// deps-lint-skip: row, col

This comment has been minimized.

Copy link
@zombieJ

zombieJ added some commits Mar 13, 2019

@ztplz

This comment has been minimized.

Copy link
Collaborator

commented Mar 13, 2019

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

@zombieJ

This comment has been minimized.

Copy link
Member Author

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 referenced this pull request Mar 14, 2019

Closed

Page Header Disables Tree Shaking #15407

1 of 1 task complete
@zombieJ

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

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

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

@afc163

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

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

@zombieJ

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

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

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

@afc163

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

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

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

@zombieJ

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

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

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

意思是不用也引进来?

@afc163 afc163 merged commit c1ef989 into master Mar 14, 2019

27 of 30 checks passed

codebeat 2 issues resolved and 6 introduced
Details
Header rules No header rules processed
Details
Pages changed 12 new files uploaded
Details
Auto Assign Auto Assign
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
CodeFactor No issues found.
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
License Compliance All checks passed.
Details
Mixed content No mixed content detected
Details
Redirect rules 18 redirect rules processed
Details
WIP Ready for review
Details
ant-design.ant-design #ant design succeeded
Details
ci/circleci: compile Your tests passed on CircleCI!
Details
ci/circleci: dist Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test_dist Your tests passed on CircleCI!
Details
ci/circleci: test_dist_15 Your tests passed on CircleCI!
Details
ci/circleci: test_dom Your tests passed on CircleCI!
Details
ci/circleci: test_dom_15 Your tests passed on CircleCI!
Details
ci/circleci: test_es Your tests passed on CircleCI!
Details
ci/circleci: test_es_15 Your tests passed on CircleCI!
Details
ci/circleci: test_lib Your tests passed on CircleCI!
Details
ci/circleci: test_lib_15 Your tests passed on CircleCI!
Details
ci/circleci: test_node Your tests passed on CircleCI!
Details
ci/circleci: test_node_15 Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 97880a9...f9d8e50
Details
codecov/project 94.04% (-0.07%) compared to 97880a9
Details
deploy/netlify Deploy preview ready!
Details
security/snyk - package.json (paranoidjk) No new issues
Details

@delete-merged-branch delete-merged-branch bot deleted the deps-lint branch Mar 14, 2019

@afc163

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.