-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
Add instance-properties group to sort-comp. Issue #599. #685
Conversation
Allowing it to be configurable is excellent. However, changing the default order makes it a breaking change. Should this preserve the default order, and be semver-minor, and change the default later? |
@ljharb I've reverted the default order. My initial way of thinking was that this change could be part of |
That's a fair point. @yannickcr should weigh in on that. |
Since What is the most common order used by React users here? |
Instance properties are still stage 1, so we don't use them yet at Airbnb. However, since instance properties run almost as if they were in the constructor, i think they absolutely should all be placed just like the constructor - at the top. |
This has become a common pattern for the react community, and one that I would certainly like to enforce in my projects. Mind taking another look at this and weighing in on why it is not accepted? (other than the conflicts to be resolved now) |
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.
It would be great to get a rebase of this.
2. lifecycle methods: `displayName`, `propTypes`, `contextTypes`, `childContextTypes`, `mixins`, `statics`,`defaultProps`, `constructor`, `getDefaultProps`, `getInitialState`, `state`, `getChildContext`, `componentWillMount`, `componentDidMount`, `componentWillReceiveProps`, `shouldComponentUpdate`, `componentWillUpdate`, `componentDidUpdate`, `componentWillUnmount` (in this order). | ||
3. custom methods | ||
4. `render` method | ||
2. instance properties |
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.
It looks like this documentation needs to be updated, since the default order has not been modified yet.
It would be nice to open a PR that updates the default and documentation that we could bring in as part of v7.0.0.
Public Class Fields are on stage 2 already, why it still hasn't been merged into |
@klimashkin this needs reviews from more collabs first, and it needs a rebase. Separately, stage 3 is the time when it's safe to start using a feature, so that's when it will become critical to be supported here. |
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.
Default configuration should not be updated in the documentation (see @lencioni comment) and a rebase is needed.
Class fields is now a stage 3 proposal. Happy to contribute if this needs someone else to take it on. |
Unfortunately @le0nik deleted their fork, so this PR can never land. |
Relevant issue: #599
It's a common convention to have instance properties on top(right below the static properties/methods), like this:
Right now
sort-comp
rule doesn't allow that and wantscount
to be belowcomponentDidMount
, unless you specify it by name in the configuration.This PR's aim is to allow placing instance properties separately.
The default configuration is not changed.