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

Update isReferentiallyTransparentFunctionComponent.js #275

Merged
merged 1 commit into from
Nov 18, 2016
Merged

Update isReferentiallyTransparentFunctionComponent.js #275

merged 1 commit into from
Nov 18, 2016

Conversation

oliviertassinari
Copy link
Contributor

@oliviertassinari oliviertassinari commented Nov 11, 2016

I'm wondering about the purpose of !Component.propTypes.
Using propTypes is quite a popular pattern.
If it's only here in order to get proper warning in dev mode, that shouldn't prevent us from
instantiation less react components in production.

Hopefully, we can use babel-plugin-transform-react-remove-prop-types to increase the number of referentially transparent components

@wuct
Copy link
Contributor

wuct commented Nov 12, 2016

If we use the babel plugin to remove propTypes in production, there is no need to check the environment here again, right?

@oliviertassinari
Copy link
Contributor Author

oliviertassinari commented Nov 12, 2016

If we use the babel plugin to remove propTypes in production

  • Removing 100% of the propTypes in production is quite challenging. You need either lib author do wrap their propTypes with a process.env.NODE_ENV === 'production' or make babel parse the all node_modules folder. Not that easy.
  • It's a micro-optimization, but even if you remove 100% of the propTypes, I think that checking !Component.propTypes is a lost CPU cycle in production.

@wuct
Copy link
Contributor

wuct commented Nov 12, 2016

I get your point now. IMO !Component.propTypes should be removed in production, because React doesn't check propTypes in production.

@@ -5,7 +5,7 @@ const isReferentiallyTransparentFunctionComponent = Component => Boolean(
!isClassComponent(Component) &&
!Component.defaultProps &&
!Component.contextTypes &&
!Component.propTypes
(!Component.propTypes || process.env.NODE_ENV === 'production')
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this line be considered dead code and be eliminated in production?

Copy link
Contributor

Choose a reason for hiding this comment

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

swapping on process.env.NODE_ENV === 'production' || !Component.propTypes will avoid getting the propTypes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@istarkov I agree that it would be better. I believe that uglify-js is smart enough to remove those if (!foo.bar || true) expression but that may not be true with other minifier, they may keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I've just tried UglifyJS, both true || foo and foo || true work.

I'm wondering about the purpose of `!Component.propTypes`.
Using `propTypes` is quite a popular pattern.
If it's only here in order to get proper warning in dev mode, that shouldn't prevent us from
instantiation less react components.

Hopefully, we can use [babel-plugin-transform-react-remove-prop-types](https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types) to increase the number of referentially transparent components
@wuct
Copy link
Contributor

wuct commented Nov 18, 2016

LGTM. I'd like to know how @acdlite and @istarkov think.

@istarkov
Copy link
Contributor

I like this PR 👍 to merge

@wuct wuct merged commit 8b8ca70 into acdlite:master Nov 18, 2016
@wuct
Copy link
Contributor

wuct commented Nov 18, 2016

@oliviertassinari thanks!

@oliviertassinari oliviertassinari deleted the patch-2 branch November 18, 2016 13:15
@oliviertassinari
Copy link
Contributor Author

Awesome, thanks 🎉 !

Any plan to do a release? The last one was 144 days ago.
We use the decoration pattern quite extensively at @doctolib.
Performance is important for us. @neoziro is even investigating toward combining all those HoCs into a single one with recompact while keeping the same API.

@gregberge
Copy link

I agree with @oliviertassinari, the project is moving slowly, that's why we are building "recompact" apart recompose. We will switch to it and use it in production. If the experimentation is a success, we could consider merging the two projects in the future.

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