-
-
Notifications
You must be signed in to change notification settings - Fork 26.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removing propTypes in production build? #209
Comments
@spicyj Do you think this is reasonable for most apps? |
Seems like a good idea to me. |
Related: it would be cool if wrapping 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! |
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 |
I think it’s not a big deal because React will warn very soon when you attempt to call |
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? |
A linter against reading it? |
I think there are some valid use cases for accessing it, like DangerButton.propTypes = {
...Button.propTypes,
somethingElse: ...
} |
React-Bootstrap uses It's a fine optimization in most cases but it'd cause difficult-to-diagnose errors if you were to run it against |
We wouldn’t touch 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.) |
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 Looks hacky but it's fairly DRY and has worked well in practice. |
Thanks for the info! Looks indeed hacky, but if libraries rely on component |
I guess? It's a shame, though – that's a really cool transform, and I'm going to look at using it for production app builds now that I know about it. |
OK, I don't think the size reduction is worth the surprising behavior here. |
Quite glad you went with this decision - the performance benefit is likely relatively nonexistent and the size benefit seems questionable to me (if you really need to save space that badly, composing For my library I'm exposing an extendable class I'm calling class PurePropTypesComponent extends React.Component {
shouldComponentUpdate (nextProps, nextState) {
if (this.props) {
// for props, only shallow compare keys found in propTypes
for (const propName of Object.keys(this.constructor.propTypes || {})) {
if (this.props[propName] !== nextProps[propName]) {
return true;
}
}
}
if (this.state) {
// for state, do normal shallow compare
for (const key of Object.keys(this.state)) {
if (!(key in nextState)) {
return true;
}
}
for (const key of Object.keys(nextState)) {
if(!(this.state[key] === nextState[key])) {
return true;
}
}
}
return false;
}
} If a user stripped ETA: however reading this discussion convinces me a higher-order component might be better/more opt-outable implementation option for me. |
To be honest I feel like it might have been better to actually enforce stripping We definitely don’t recommend relying on |
Aha - sorry I brought it up :P Good to know though - not sure what's so bad about relying on |
It’s just absolutely not obvious that something that’s used for one purpose (DEV-only type validation) is also being used for some other purpose. People with experience of working on React projects would be confused if they bumped into a project where it mattered.
Why are they receiving props they aren’t consuming? If you want |
Im not building react router, but consider React Router, whose |
If that component doesn't care about those props and wants to short-circuit rendering even if some of them changes, it can render another component which is just a function MyRoute(props) {
return <MyPureRoute propICareAbout={props.propICareAbout} />
}
class MyPureRoute extends React.PureComponent {
// ...
}
export default MyRoute |
Nice - this is a solution I hadn't considered in this instance. However my fear is this kind of strips the ease of use I had associated with my original solution. It would be possible to implement a higher order component whose creator function takes a I guess if |
Would this solve the problem? const devProps = (component, props) =>
process.env.NODE_ENV === 'development' && (component.propTypes = props)
devProps(MyComp, {
someProp: PropTypes.string
}) |
No because most dead-code elimination tools (including Uglify that we use) aren't smart enough to process this. Maybe Google Closure Compiler could do this but it’s too hard to integrate for CRA users. So, does anyone still want to send a PR? I think we’d take it for 2.0. |
Is this just a matter of applying |
I think so, hopefully. |
(or rather in our Babel preset) |
Ah, okay. It looks like that plugin also has a couple of options to consider. For example, do we want to strip I'll put together a PR. |
Not from |
Some library authors are using the "wrap" mode of babel-plugin-transform-react-remove-prop-types to get the |
Yeah – discretion there should be left to library authors for now. |
The |
Aren’t propTypes required for anything using context? |
No, |
* Remove PropTypes from production build (facebook#209) * Added react/forbid-foreign-prop-types rule to eslint config * Removed react/forbid-foreign-prop-types rule from eslint config
What do you think about removing the
propTypes
in the production build to reduce the bundle size usingbabel-plugin-transform-react-remove-prop-types
?I've already prepared a fork, so if you think it is a good idea, I can submit a pull request.
The text was updated successfully, but these errors were encountered: