Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Use eager factory optimization only in production env (close #442) #473

Merged
merged 3 commits into from
Aug 15, 2017
Merged

Use eager factory optimization only in production env (close #442) #473

merged 3 commits into from
Aug 15, 2017

Conversation

deepsweet
Copy link
Contributor

As discussed in #442.

  • We don't care about propTypes check anymore since it's already no-op in prod env.
  • Notice few shallowmount changes in tests – that's actually the confusion which I described in corresponding issue.

✌️

@codecov-io
Copy link

codecov-io commented Aug 14, 2017

Codecov Report

Merging #473 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #473   +/-   ##
=======================================
  Coverage   88.86%   88.86%           
=======================================
  Files          53       53           
  Lines         386      386           
=======================================
  Hits          343      343           
  Misses         43       43
Impacted Files Coverage Δ
...ose/isReferentiallyTransparentFunctionComponent.js 100% <ø> (ø) ⬆️
...packages/recompose/utils/createEagerElementUtil.js 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 828c36e...4a8535c. Read the comment docs.

@wuct
Copy link
Contributor

wuct commented Aug 15, 2017

The title of this PR and the commit should be "Use eager factory optimization only in production env ".

@deepsweet deepsweet changed the title Use eager factory optimization only in non-production env (close #442) Use eager factory optimization only in production env (close #442) Aug 15, 2017
@deepsweet
Copy link
Contributor Author

@wuct oops, fixed.

@wuct
Copy link
Contributor

wuct commented Aug 15, 2017

Looks good to me, thanks!

@wuct
Copy link
Contributor

wuct commented Aug 15, 2017

Hmmm, actually I think we should move the env check out of isReferentiallyTransparentFunctionComponent(), otherwise there is an abstraction leak.

@deepsweet
Copy link
Contributor Author

Agreed. So both to createEagerFactory and createEagerElement?

@deepsweet
Copy link
Contributor Author

Hm, looks like createEagerElementUtil is the right place.

@deepsweet
Copy link
Contributor Author

Done.

I returned check for propTypes back because it's actually a part of abstraction, so this way it's not related to any env and concepts are divided. But optimization will not work even in production env if you have propTypes.

Options:

  1. Remove check for propTypes from isReferentiallyTransparentFunctionComponent at all.
  2. I'd like to work on something similar to babel-plugin-transform-react-remove-prop-types but for setPropTypes, I think it's a more proper way to achieve prod-only optimization.

WDYT?

@@ -4,9 +4,9 @@ const isReferentiallyTransparentFunctionComponent = Component =>
Boolean(
typeof Component === 'function' &&
!isClassComponent(Component) &&
!Component.propTypes &&
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like break previous behaviour?

@istarkov
Copy link
Contributor

istarkov commented Aug 15, 2017

IMO option 1 is preferred, having that it is fully compatible with previous behaviour in production mode.

@deepsweet
Copy link
Contributor Author

Done.

@deepsweet
Copy link
Contributor Author

isReferentiallyTransparentFunctionComponent returns true for functions that use propTypes

@istarkov
Copy link
Contributor

Thank you

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants