Skip to content

Commit

Permalink
Add ES6 methods to sort-comp default configuration (fixes #97, fixes #…
Browse files Browse the repository at this point in the history 
…122)
  • Loading branch information
@yannickcr
yannickcr committed Jun 23, 2015
1 parent f6292bb commit d657ea5
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 3 deletions.
12 changes: 9 additions & 3 deletions docs/rules/sort-comp.md
View file
Expand Up @@ -6,9 +6,10 @@ When creating React components it is more convenient to always follow the same o

With default configuration the following organisation must be followed:

1. lifecycle methods: `displayName`, `propTypes`, `contextTypes`, `childContextTypes`, `mixins`, `statics`, `getDefaultProps`, `getInitialState`, `getChildContext`, `componentWillMount`, `componentDidMount`, `componentWillReceiveProps`, `shouldComponentUpdate`, `componentWillUpdate`, `componentDidUpdate`, `componentWillUnmount` (in this order).
2. custom methods
3. `render` method
1. `constructor` method
2. lifecycle methods: `displayName`, `propTypes`, `contextTypes`, `childContextTypes`, `mixins`, `statics`,`defaultProps`, `getDefaultProps`, `getInitialState`, `getChildContext`, `componentWillMount`, `componentDidMount`, `componentWillReceiveProps`, `shouldComponentUpdate`, `componentWillUpdate`, `componentDidUpdate`, `componentWillUnmount` (in this order).
3. custom methods
4. `render` method

The following patterns are considered warnings:

Expand Down Expand Up @@ -51,6 +52,7 @@ The default configuration is:
```js
{
order: [
'constructor',
'lifecycle',
'everything-else',
'render'
Expand All @@ -63,6 +65,7 @@ The default configuration is:
'childContextTypes',
'mixins',
'statics',
'defaultProps',
'getDefaultProps',
'getInitialState',
'getChildContext',
Expand All @@ -78,6 +81,7 @@ The default configuration is:
}
```

* `constructor` is refering to the `constructor` method.
* `lifecycle` is refering to the `lifecycle` group defined in `groups`.
* `everything-else` is a special group that match all the methods that do not match any of the other groups.
* `render` is refering to the `render` method.
Expand All @@ -89,6 +93,7 @@ For example, if you want to place your event handlers (`onClick`, `onSubmit`, et
```js
"react/sort-comp": [1, {
order: [
'constructor',
'lifecycle',
'/^on.+$/',
'render',
Expand Down Expand Up @@ -124,6 +129,7 @@ If you want to split your `render` method into smaller ones and keep them just b
```js
"react/sort-comp": [1, {
order: [
'constructor',
'lifecycle',
'everything-else',
'rendering',
Expand Down
2 changes: 2 additions & 0 deletions lib/rules/sort-comp.js
View file
Expand Up @@ -48,6 +48,7 @@ module.exports = function(context) {

var methodsOrder = getMethodsOrder({
order: [
'constructor',
'lifecycle',
'everything-else',
'render'
Expand All @@ -60,6 +61,7 @@ module.exports = function(context) {
'childContextTypes',
'mixins',
'statics',
'defaultProps',
'getDefaultProps',
'getInitialState',
'getChildContext',
Expand Down

4 comments on commit d657ea5

@mathieumg
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have thought the constructor would be better suited below the static properties, but above the lifecycle methods.

@yannickcr
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually we still only check the property name, not the type (static, get, set) issue #100.

Here statics only match the statics method in ES5 classes.

@mathieumg
Copy link
Contributor

Choose a reason for hiding this comment

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

I know, but we know defaultProps, statics, mixins, etc. will always be defined as static class properties, no? While it can be argued that constructor is not a React lifecycle method per-se, I think the following would be preferable:

order: [
  'lifecycle',
  'everything-else',
  'render'
],
groups: {
  lifecycle: [
     'displayName',
     'propTypes',
     'contextTypes',
     'childContextTypes',
     'mixins',
     'statics',
     'defaultProps',
     'constructor',
     'getDefaultProps',
         // etc.
  ]
}

This way, this:

class MyComponent extends React.Component {
  static propTypes = {};
  static defaultProps = {};

  constructor() {/* ... */}

  render()  {/* ... */}
}

feels cleaner than:

class MyComponent extends React.Component {
  constructor() {/* ... */}

  static propTypes = {};
  static defaultProps = {};

  render()  {/* ... */}
}

@yannickcr
Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I understand. It feels cleaner to me too.

we know defaultProps, statics, mixins, etc. will always be defined as static class properties, no?

I'm not working in ES6 and I am not very familiar with it yet, but for now I've seen 3 different patterns:

ES6 instance property :

class Hello extends React.Component {
  // ...
}
Hello.displayName = 'Hello';

ES6 class method:

class Hello extends React.Component {
  static get displayName() {
    return 'Hello';
  }
  // ...
}

ES7 class property:

class Hello extends React.Component {
  static displayName = 'Hello'
  // ...
}

In second case the method type can be static, get, static get or none. In third case the property type can be static or none. The static type does not seems to be mandatory.

However, I'd really like to come out with a good default configuration for ES5/ES6 classes, with method type support if needed.

Please sign in to comment.