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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use mounted property, replace mounted state, resolves #16290 #16322

Closed
wants to merge 2 commits into from
Closed

fix: use mounted property, replace mounted state, resolves #16290 #16322

wants to merge 2 commits into from

Conversation

rinick
Copy link
Contributor

@rinick rinick commented Apr 25, 2019

馃 This is a ...

  • Bug fix

馃懟 What's the background?

resolves issue : destroyed Menu right after it's created will cause React warning #16290

馃挕 Solution

use mounted property instead of mounted state

鈽戯笍 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 Apr 26, 2019

Deploy preview for ant-design ready!

Built with commit e1f62ac

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

@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

Merging #16322 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16322      +/-   ##
==========================================
+ Coverage   95.28%   95.35%   +0.07%     
==========================================
  Files         253      253              
  Lines        6780     6776       -4     
  Branches     1956     1968      +12     
==========================================
+ Hits         6460     6461       +1     
+ Misses        319      314       -5     
  Partials        1        1
Impacted Files Coverage 螖
components/menu/index.tsx 97.34% <100%> (-0.1%) 猬囷笍
components/_util/wave.tsx 88.46% <0%> (+2.88%) 猬嗭笍
components/_util/openAnimation.tsx 75% <0%> (+7.14%) 猬嗭笍

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 685f433...e1f62ac. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

Merging #16322 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16322      +/-   ##
==========================================
+ Coverage   95.28%   95.35%   +0.07%     
==========================================
  Files         253      253              
  Lines        6780     6776       -4     
  Branches     1956     1968      +12     
==========================================
+ Hits         6460     6461       +1     
+ Misses        319      314       -5     
  Partials        1        1
Impacted Files Coverage 螖
components/menu/index.tsx 97.34% <100%> (-0.1%) 猬囷笍
components/_util/wave.tsx 88.46% <0%> (+2.88%) 猬嗭笍
components/_util/openAnimation.tsx 75% <0%> (+7.14%) 猬嗭笍

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 685f433...e1f62ac. Read the comment docs.

@afc163 afc163 requested a review from zombieJ April 26, 2019 08:00
@afc163 afc163 changed the title use mounted property, replace mounted state, resolves #16290 fix: use mounted property, replace mounted state, resolves #16290 Apr 26, 2019
@zombieJ
Copy link
Member

zombieJ commented Apr 28, 2019

This should fix in raf instead of change to this.mounted = xxx.

@zombieJ
Copy link
Member

zombieJ commented Apr 28, 2019

close since #16370

@zombieJ zombieJ closed this Apr 28, 2019
@rinick
Copy link
Contributor Author

rinick commented Apr 28, 2019

This should fix in raf instead of change to this.mounted = xxx.

@zombieJ
would this still cause an unnecessary render() ?
I think the whole point of using state is because we want to let react framework decide when to render.
if set mounted=true doesn't require a re-render, then there is no reason to use state

@zombieJ
Copy link
Member

zombieJ commented Apr 29, 2019

Since React will release concurrent mode in ~Q2 2019, we use this.state instead of this.[variable] to ensure it not breaks in concurrent mode.
I know use this.[variable] is still safe in some condition, but use pure state is much easier.

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.

None yet

2 participants