You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Related: it would be cool if wrapping propType declarations in an if (process.env.NODE_ENV !== 'production') block became a common thing to make this automatic for your own code and particularly for your dependencies, for which you're probably using the transpiled ES5 they published to npm - is there a plugin for that?
We screwed up and didn't remove propTypes in dev when we started and now people rely on it existing in their app, so doing it now would be a big breaking change.
But for new apps that would be a good default imo. Now, it may be confusing that it is removed in this starter kit and not anywhere else
Now, it may be confusing that it is removed in this starter kit and not anywhere else
I think it’s not a big deal because React will warn very soon when you attempt to call PropTypes validators manually: facebook/react#7132. So people will know they’re not meant to be used in production, whether or not they use this kit.
ForbesLindesay, aweary, rmariuzzo and arturoromeroslc
This would probably be fine. I can imagine some people using it to mask the props object but hopefully not many. Can we add a linter against .propTypes?
React-Bootstrap uses propTypes to mask props objects extensively, and a number of the components there would break outright if you stripped out propTypes in node_modules.
It's a fine optimization in most cases but it'd cause difficult-to-diagnose errors if you were to run it against node_modules.
Thanks for the info! Looks indeed hacky, but if libraries rely on component propTypes, user code can rely on them as well and removing them can lead to unexpected behaviour in production. Seems like this can be closed then, right?
The wrap mode would break for libs that enumerate propTypes to do filtering etc. I feel comfortable with asking people not to do this in their own apps, but I'm wary of breaking third-party code like this where there might not be an easy fix.
Activity
[-]Removing propTypes in production build[/-][+]Removing propTypes in production build?[/+]gaearon commentedon Jul 26, 2016
@spicyj Do you think this is reasonable for most apps?
timche commentedon Jul 26, 2016
For reference:
The advantage of this transform is saving bandwidth. We are not aware of any limitation.
ForbesLindesay commentedon Jul 26, 2016
Seems like a good idea to me.
insin commentedon Jul 26, 2016
Related: it would be cool if wrapping
propType
declarations in anif (process.env.NODE_ENV !== 'production')
block became a common thing to make this automatic for your own code and particularly for your dependencies, for which you're probably using the transpiled ES5 they published to npm - is there a plugin for that?Edit: Created an issue here: oliviertassinari/babel-plugin-transform-react-remove-prop-types#35 - would be amazing to land this change and promote its use throughout the React ecosystem!
vjeux commentedon Jul 26, 2016
We screwed up and didn't remove propTypes in dev when we started and now people rely on it existing in their app, so doing it now would be a big breaking change.
But for new apps that would be a good default imo. Now, it may be confusing that it is removed in this starter kit and not anywhere else
gaearon commentedon Jul 26, 2016
I think it’s not a big deal because React will warn very soon when you attempt to call
PropTypes
validators manually: facebook/react#7132. So people will know they’re not meant to be used in production, whether or not they use this kit.sophiebits commentedon Jul 26, 2016
This would probably be fine. I can imagine some people using it to mask the props object but hopefully not many. Can we add a linter against .propTypes?
gaearon commentedon Jul 26, 2016
A linter against reading it?
gaearon commentedon Jul 26, 2016
I think there are some valid use cases for accessing it, like
taion commentedon Jul 26, 2016
React-Bootstrap uses
propTypes
to mask props objects extensively, and a number of the components there would break outright if you stripped outpropTypes
innode_modules
.It's a fine optimization in most cases but it'd cause difficult-to-diagnose errors if you were to run it against
node_modules
.gaearon commentedon Jul 26, 2016
We wouldn’t touch
node_modules
, just the user code.On the other hand there’s no reason why somebody shouldn’t be able to copy and paste a component from React Bootstrap and tweak it.
So I guess as long as people rely on this, we can’t do it. (Unless we specifically lint against it.)
taion commentedon Jul 26, 2016
FWIW, this specific use case in React-Bootstrap is something like this: https://github.com/react-bootstrap/react-bootstrap/blob/v0.30.0/src/DropdownButton.js#L26-L27
We have parent components that render two child components, and we use
propTypes
to split out which prop goes where.Looks hacky but it's fairly DRY and has worked well in practice.
timche commentedon Jul 26, 2016
Thanks for the info! Looks indeed hacky, but if libraries rely on component
propTypes
, user code can rely on them as well and removing them can lead to unexpected behaviour in production. Seems like this can be closed then, right?37 remaining items
gaearon commentedon Jan 17, 2018
The
wrap
mode would break for libs that enumeratepropTypes
to do filtering etc. I feel comfortable with asking people not to do this in their own apps, but I'm wary of breaking third-party code like this where there might not be an easy fix.quantizor commentedon Jan 17, 2018
Aren’t propTypes required for anything using context?
gaearon commentedon Jan 17, 2018
No,
contextTypes
/childContextTypes
are. Those aren't removed as far as I can tell.Remove PropTypes from production build (#209) (#3818)
Remove PropTypes from production build (facebook#209) (facebook#3818)