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

build: add strictNullCheck config #2977

Merged
merged 1 commit into from
Mar 12, 2019
Merged

Conversation

wzhudev
Copy link
Member

@wzhudev wzhudev commented Feb 27, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[x] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

@wzhudev
Copy link
Member Author

wzhudev commented Feb 27, 2019

@wilsoncook @cipchk @vthinkxie @hsuanxyz @simplejason

Please review code of the components you are in charge of. In case of any state related bug introduced.

Notes for further development:

  1. Pay attention to properties that can be set to null or undefined. Try to refactor again in milestone 2.
  2. When you want to get a form field, you have to assert that you can get it. Like formGroup.get('name')!.hasError. In template you have to use formGroup.get('name')?.hasError.
  3. When a function parameter is optional, pass undefined instead of null.
  4. Pay attention to optional declaration of a property. ?:
  5. Avoid to declare a property as someOtherType | null, ?:someOtherType is preferred.
  6. For subscriptions that need to be unsubscribe when component destroys, ! is free to use.
  7. In test, native API like querySelector may returns null. You can add an assertion.
  8. Use helpers like Required<> Optional<>.

Modules acquire additional attention:

  1. Date Picker, especially helper class CandyDate.
  2. Upload, especially relate to File.
  3. Tree.

@vthinkxie vthinkxie mentioned this pull request Feb 27, 2019
7 tasks
Copy link
Member

@cipchk cipchk left a comment

Choose a reason for hiding this comment

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

我提了几处语法建议,我不确定这些变更是否可能会影响到一些测试用例。

components/affix/nz-affix.component.ts Outdated Show resolved Hide resolved
components/anchor/nz-anchor.component.ts Outdated Show resolved Hide resolved
components/anchor/nz-anchor.component.ts Outdated Show resolved Hide resolved
components/affix/nz-affix.component.ts Show resolved Hide resolved
@wzhudev
Copy link
Member Author

wzhudev commented Feb 28, 2019

@simplejason 我注意到 nz-tree-base 的 formatEvent 方法返回的 NzFormatEmitEvent 被各个 subject 用来 emit 事件, 但是不同 subject emit 的事件对于字段的 optional 要求是不一样的, 我添加了一些 Required (typescript 的类型修饰符), 请检查一下会不会有问题.

另外请加查一下 NzTreeNode 各个字段是否是必须的, 在不带来 break change 的前提下, ! 用的少一些就还是好一些.

@wzhudev wzhudev force-pushed the tsconfig-null branch 3 times, most recently from 57665be to 26a91f6 Compare March 1, 2019 05:57
@wzhudev wzhudev marked this pull request as ready for review March 1, 2019 05:57
@netlify
Copy link

netlify bot commented Mar 1, 2019

Deploy preview for ng-zorro-master ready!

Built with commit 26a91f6

https://deploy-preview-2977--ng-zorro-master.netlify.com

@netlify
Copy link

netlify bot commented Mar 1, 2019

Deploy preview for ng-zorro-master ready!

Built with commit 03aa3bf

https://deploy-preview-2977--ng-zorro-master.netlify.com

@codecov
Copy link

codecov bot commented Mar 1, 2019

Codecov Report

Merging #2977 into master will increase coverage by <.01%.
The diff coverage is 95.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2977      +/-   ##
==========================================
+ Coverage   97.32%   97.32%   +<.01%     
==========================================
  Files         549      550       +1     
  Lines       11489    11495       +6     
  Branches      815      817       +2     
==========================================
+ Hits        11182    11188       +6     
- Misses        197      199       +2     
+ Partials      110      108       -2
Impacted Files Coverage Δ
...s/date-picker/lib/decade/decade-panel.component.ts 100% <ø> (ø) ⬆️
components/select/nz-option.pipe.ts 100% <ø> (ø) ⬆️
components/slider/nz-slider-definitions.ts 100% <ø> (ø) ⬆️
components/slider/nz-slider-track.component.ts 100% <ø> (ø) ⬆️
...nts/date-picker/lib/month/month-table.component.ts 100% <ø> (ø) ⬆️
components/carousel/nz-carousel.component.ts 93.54% <ø> (ø) ⬆️
...nents/date-picker/lib/year/year-panel.component.ts 100% <ø> (ø) ⬆️
components/message/nz-message.service.ts 97.91% <ø> (ø) ⬆️
components/dropdown/nz-dropdown.service.ts 100% <ø> (ø) ⬆️
...mponents/notification/nz-notification.component.ts 100% <ø> (ø) ⬆️
... and 85 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 c598214...03aa3bf. Read the comment docs.

@vthinkxie vthinkxie merged commit 5bd494b into NG-ZORRO:master Mar 12, 2019
@wzhudev wzhudev deleted the tsconfig-null branch March 12, 2019 09:36
Ricbet pushed a commit to Ricbet/ng-zorro-antd that referenced this pull request Apr 9, 2020
hsuanxyz pushed a commit to hsuanxyz/ng-zorro-antd that referenced this pull request Aug 5, 2020
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.

5 participants