Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

perf(renderComponent): drop FelaTheme and use React.Context directly #1163

Merged
merged 3 commits into from
Apr 9, 2019

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Apr 4, 2019

This PR implements findings from #1106.

Goal

We want to get rid of FelaTheme as it produces two components in the tree and slow downs rendering.

How

We have two use cases for renderComponent(): UIComponent and createComponent().

createComponent() allows to use only functions, so we can use useContext() hook.
UIComponent is a class component, we can't use hooks inside them, but there is Class.contextType that can be adopted for this case.

Measures

See for more details #1106. Raw measures ContextUse and ContextClassField are proposed approaches.

Example Avg Avg diff Median Median diff
ContextUse.perf.tsx 44.76 +2.22% 38.98 38.98
ContextConsumer.perf.tsx 52.94 +20.9% 48.01 +23.17%
ContextClassConsumer.perf.tsx 63.73 +45.54% 58.06 +48.95%
ContextClassField.perf.tsx 55.79 +27.4% 51.22 +31.4%

From real life

I used #1006 to compare.

⚛ App [mount]697.1149999881163
⚛ App [mount]735.6550000258721
⚛ App [mount]727.5300000328571
⚛ App [mount]706.754999991972
⚛ App [mount]652.7350000105798
⚛ App [mount]602.1899999468587
⚛ App [mount]626.8550000386313
⚛ App [mount]630.9650000184774

Savings are 70-100ms.

@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

Merging #1163 into master will increase coverage by 0.01%.
The diff coverage is 96.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1163      +/-   ##
==========================================
+ Coverage   82.46%   82.47%   +0.01%     
==========================================
  Files         740      740              
  Lines        8753     8754       +1     
  Branches     1171     1170       -1     
==========================================
+ Hits         7218     7220       +2     
  Misses       1519     1519              
+ Partials       16       15       -1
Impacted Files Coverage Δ
packages/react/src/lib/createComponent.tsx 100% <100%> (ø) ⬆️
packages/react/src/lib/UIComponent.tsx 92.3% <100%> (+0.64%) ⬆️
packages/react/src/lib/renderComponent.tsx 93.05% <95.83%> (+0.95%) ⬆️

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 0ba5c0d...9721629. Read the comment docs.

@@ -1,5 +1,8 @@
import * as React from 'react'
import * as _ from 'lodash'
// @ts-ignore We have this export in package, but it is not present in typings
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@kuzhelov kuzhelov left a comment

Choose a reason for hiding this comment

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

please, just update the CHANGELOG before merging 👍

@layershifter layershifter merged commit 783c783 into master Apr 9, 2019
@delete-merged-branch delete-merged-branch bot deleted the perf/context-update branch April 9, 2019 14:21
@kuzhelov kuzhelov mentioned this pull request Apr 18, 2019
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants