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

perf(Form): FormContext use a memoized value(#21976) #21980

Merged
merged 2 commits into from Mar 8, 2020

Conversation

@qiqiboy
Copy link
Contributor

qiqiboy commented Mar 7, 2020

🤔 This is a ...

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

🔗 Related issue link

#21976

💡 Background and solution

In current, FormContext.Provider use an inline Object variable as the value, it will always trigger unintentional renders in consumers when Form or Form's parent re-renders.

📝 Changelog

Language Changelog
🇺🇸 English FormContext use a memoized value to avoid trigger FormItem's unintentional renders
🇨🇳 Chinese FormContext使用memoized值避免FormItem产生额外的渲染

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • 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
@pr-triage pr-triage bot added the PR: unreviewed label Mar 7, 2020
@ant-design-bot

This comment has been minimized.

Copy link
Contributor

ant-design-bot commented Mar 7, 2020

@yoyo837

This comment has been minimized.

Copy link
Collaborator

yoyo837 commented Mar 7, 2020

memo 本身也消耗性能,这里好像不明显, 多产生个对象。

@qiqiboy

This comment has been minimized.

Copy link
Contributor Author

qiqiboy commented Mar 7, 2020

memo 本身也消耗性能,这里好像不明显, 多产生个对象。

正常情况下Form或者Form的上层组件render的话,所有FormItem都会重新渲染,是这么回事。这种情况下,这里的memo也确实显得是额外的比较开销。

但是当FormFormItem中间有使用shouldComponentUpdate优化的组件时,依然无法阻止FormItem的重新渲染,因为Providervalue变化,会强制更新所有连接的Consumer,即使Consumer的父级使用了shouldComponentUpdate.

虽然这种优化并不常见,4.0里的Form性能也很好了,但是这里的FormContext用法确实不符合最佳实践,破坏shouldComponentUpdate对组件渲染优化的逻辑。这里的一个小的memo应当也会影响太多,这几个值本身数据也很小,但是对于渲染的优化影响是极大的。

@@ -74,6 +74,18 @@ const InternalForm: React.ForwardRefRenderFunction<unknown, FormProps> = (props,
const [wrapForm] = useForm(form);
wrapForm.__INTERNAL__.name = name;

const formContextValue = React.memo(

This comment has been minimized.

Copy link
@zombieJ

zombieJ Mar 8, 2020

Member

useMemo

This comment has been minimized.

Copy link
@qiqiboy

qiqiboy Mar 8, 2020

Author Contributor

Sorry,已修改

qiqiboy
@codesandbox

This comment has been minimized.

Copy link

codesandbox bot commented Mar 8, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d72bc19:

Sandbox Source
antd reproduction template Configuration
@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 8, 2020

Codecov Report

Merging #21980 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #21980      +/-   ##
==========================================
+ Coverage   97.93%   97.93%   +<.01%     
==========================================
  Files         306      306              
  Lines        7024     7026       +2     
  Branches     1889     1934      +45     
==========================================
+ Hits         6879     6881       +2     
  Misses        145      145
Impacted Files Coverage Δ
components/form/Form.tsx 100% <100%> (ø) ⬆️

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 8eb6647...d72bc19. Read the comment docs.

@zombieJ

This comment has been minimized.

Copy link
Member

zombieJ commented Mar 8, 2020

memo 本身也消耗性能,这里好像不明显, 多产生个对象。

它的场景是子元素又通过 React.Memo or shouldComponentUpdate 隔离了一层 Form.Item。本身 useMemo 性能开销还行,觉得可以加。

@zombieJ zombieJ merged commit a476197 into ant-design:master Mar 8, 2020
19 checks passed
19 checks passed
LGTM analysis: JavaScript No new or fixed alerts
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
ant-design.ant-design Build #Ant Design succeeded
Details
ant-design.ant-design (site Build_Site) site Build_Site succeeded
Details
ci/circleci: check_metadata Your tests passed on CircleCI!
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_dom Your tests passed on CircleCI!
Details
ci/circleci: test_es Your tests passed on CircleCI!
Details
ci/circleci: test_lib Your tests passed on CircleCI!
Details
ci/circleci: test_node Your tests passed on CircleCI!
Details
ci/codesandbox Building packages succeeded.
Details
codecov/patch 100% of diff hit (target 97.93%)
Details
codecov/project 97.93% (+<.01%) compared to 8eb6647
Details
security/snyk (paranoidjk) All security tests have passed
Details
@pr-triage pr-triage bot added PR: merged and removed PR: unreviewed labels Mar 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.