Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Avoid setState when unmounted / Fix Flow issues #146

Merged
merged 2 commits into from
Apr 5, 2019
Merged

Avoid setState when unmounted / Fix Flow issues #146

merged 2 commits into from
Apr 5, 2019

Conversation

angus-c
Copy link
Contributor

@angus-c angus-c commented Apr 5, 2019

Fixes #142

@angus-c angus-c changed the title Don't call setState when unmounted / Fix Flow issues Avoid setState when unmounted / Fix Flow issues Apr 5, 2019
@angus-c angus-c added the bugfix label Apr 5, 2019
@codecov
Copy link

codecov bot commented Apr 5, 2019

Codecov Report

Merging #146 into master will decrease coverage by 1.49%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #146     +/-   ##
=========================================
- Coverage   49.21%   47.72%   -1.5%     
=========================================
  Files           9        9             
  Lines         128      132      +4     
  Branches       21       23      +2     
=========================================
  Hits           63       63             
- Misses         65       69      +4
Impacted Files Coverage Δ
src/with-font-loading.js 4.76% <0%> (-1.13%) ⬇️

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 027a8a9...90adff6. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Apr 5, 2019

Codecov Report

Merging #146 into master will decrease coverage by 1.49%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #146     +/-   ##
=========================================
- Coverage   49.21%   47.72%   -1.5%     
=========================================
  Files           9        9             
  Lines         128      132      +4     
  Branches       21       23      +2     
=========================================
  Hits           63       63             
- Misses         65       69      +4
Impacted Files Coverage Δ
src/with-font-loading.js 4.76% <0%> (-1.13%) ⬇️

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 027a8a9...90adff6. Read the comment docs.

if (this.state.$fontStyles.fontFamily !== fontName) {
// $FlowFixMe
// $FlowFixMe (this can only be browser so will not be undefined)
Copy link
Member

Choose a reason for hiding this comment

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

If you did want to get rid of this, Flow's type refinement may help here:

if(loadFont) {
   loadFont(...);
}

Either seems reasonable to me though, given this is a limitation of Flow + isomorphic code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that would work because the function is

export default function(font: string) {
  if (__BROWSER__ && document) {
    // $FlowFixMe
    return document.fonts && typeof document.fonts.load === 'function'
      ? document.fonts.load(`1em ${font}`) // native API requires size
      : loadFontPolyfill(font);
  }
}

i.e. the function is always there but if NODE it returns undefined

@angus-c angus-c merged commit 1db81dd into master Apr 5, 2019
@old-fusion-bot
Copy link

old-fusion-bot bot commented Apr 5, 2019

Triggered Fusion.js build verification: https://buildkite.com/uberopensource/fusion-release-verification/builds/1853

@old-fusion-bot old-fusion-bot bot deleted the mount branch April 5, 2019 23:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants