-
Notifications
You must be signed in to change notification settings - Fork 93
Emit class components to circumvent defaultProps
warnings
#131
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
base: master
Are you sure you want to change the base?
Conversation
react is deprecating defaultProps, so we'll emit class components instead to avoid it
src/index.js
Outdated
var SVG_NAME = function SVG_NAME(props) { React.PureComponent.call(this, props); }; | ||
SVG_NAME.prototype = Object.create(React.PureComponent.prototype); | ||
SVG_NAME.prototype.constructor = SVG_NAME; | ||
SVG_NAME.prototype.render = function render() { var props = this.props; return SVG_CODE; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems very strange; only class components are pure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdym? whats expected here? 🤔 want me to change it to React.Component
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik this is a class component, only thing its not doing its being strict about how babel compiles a class to a function (eg, doing constructor call checks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not using class
syntax, then i'd expect it to use https://www.npmjs.com/package/create-react-class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh.. nice, react big monorepo without docs with the only docs linked being broken..
I'll assume this is the same createReactClass and rely on old docs and use it then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb, I've made the switch, can you check the current changes now?
I'm changing the code emission so that it emits class components so that we don't get
defaultProps
deprecated warnings anymoreThis is all based on this suggestion here #129 (comment) but since that would change the fundamental idea behind the PR (still emit functions but use assign to merge defaultProps + use a flag at the plugin level to preserve behavior), I'm opening a new one
And to make it clear: we're aware that, at least in NextJS, SSR does not support class components, but we'll consider that a fault of NextJS itself.