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

New: no-class rule (fixes #2097) #2096

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@emmenko

emmenko commented Mar 18, 2015

Hi everybody,

this is my first attempt to add a new rule, so any feedback / help is much appreciated :)

The aim of the rule is to forbid the use of class (ES6) to encourage other ways to do inheritance.

Thanks in advance

@emmenko emmenko changed the title from New rule: no-class to New: no-class rule Mar 18, 2015

@lo1tuma

This comment has been minimized.

Show comment
Hide comment
@lo1tuma

lo1tuma Mar 18, 2015

Member

I really want to have this rule in ESLint 👍.

We require an open issue for every pull request and the commit messages have to be in a specific format. Please take a look at our contribution guidelines

Member

lo1tuma commented Mar 18, 2015

I really want to have this rule in ESLint 👍.

We require an open issue for every pull request and the commit messages have to be in a specific format. Please take a look at our contribution guidelines

@emmenko

This comment has been minimized.

Show comment
Hide comment
@emmenko

emmenko Mar 18, 2015

@lo1tuma yeah, I read that (almost :) ) but I missed the create issue part. Will do that now.

PS: already signed to CLA ;)

emmenko commented Mar 18, 2015

@lo1tuma yeah, I read that (almost :) ) but I missed the create issue part. Will do that now.

PS: already signed to CLA ;)

@emmenko emmenko changed the title from New: no-class rule to New: no-class rule (fixes #2097) Mar 18, 2015

@emmenko

This comment has been minimized.

Show comment
Hide comment
@emmenko

emmenko Mar 18, 2015

@lo1tuma should be ok now (issue + commit message)

emmenko commented Mar 18, 2015

@lo1tuma should be ok now (issue + commit message)

@lo1tuma

This comment has been minimized.

Show comment
Hide comment
@lo1tuma

lo1tuma Mar 18, 2015

Member

@emmenko The commit message still needs a tag at the beginning. In your case it should be: New: <your commit message> (fixes #issue-id).

Member

lo1tuma commented Mar 18, 2015

@emmenko The commit message still needs a tag at the beginning. In your case it should be: New: <your commit message> (fixes #issue-id).

@emmenko

This comment has been minimized.

Show comment
Hide comment
@emmenko

emmenko Mar 18, 2015

@lo1tuma my bad, sorry. How about now?

emmenko commented Mar 18, 2015

@lo1tuma my bad, sorry. How about now?

Show outdated Hide outdated docs/rules/no-class.md Outdated
Show outdated Hide outdated lib/rules/no-class.js Outdated
@lo1tuma

This comment has been minimized.

Show comment
Hide comment
@lo1tuma

lo1tuma Mar 18, 2015

Member

Overall it looks good, I’ve just added a few more comments.

Member

lo1tuma commented Mar 18, 2015

Overall it looks good, I’ve just added a few more comments.

@emmenko

This comment has been minimized.

Show comment
Hide comment
@emmenko

emmenko Mar 18, 2015

@lo1tuma made adjustments according to your comments, thanks 👍

emmenko commented Mar 18, 2015

@lo1tuma made adjustments according to your comments, thanks 👍

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 18, 2015

Member

We still need a rationalization for this rule. There is no consensus on classes being a "bad part", and I see no further rationalization in the issue.

You can already disable classes using ecmaFeatures anyway, so I'm not even sure this is useful.

Member

nzakas commented Mar 18, 2015

We still need a rationalization for this rule. There is no consensus on classes being a "bad part", and I see no further rationalization in the issue.

You can already disable classes using ecmaFeatures anyway, so I'm not even sure this is useful.

@emmenko

This comment has been minimized.

Show comment
Hide comment
@emmenko

emmenko Mar 18, 2015

@nzakas well, disabling classes in the ecmaFeatures configuration doesn't do anything (that's also why I decided to add the rule). Maybe is it a bug?

$ cat test-class.js
class Foo {
  constructor() {
    this.foo = 'bar'
  }
}

$ cat .eslintrc
/* http://eslint.org/docs/rules/ */
{
  "parser": "babel-eslint",
  "ecmaFeatures": {
    "arrowFunctions": true,
    "blockBindings": true,
    "classes": false,
    "defaultParams": true,
    "jsx": true,
    "templateStrings": true
  },
  "env": {
    "browser": true,
    "jasmine": true,
    "node": true
  },
  "rules": {
    "camelcase": 2,
    "consistent-return": 2,
    "curly": [2, "multi"],
    "eol-last": 2,
    "eqeqeq": 2,
    "new-cap": [2, {"capIsNew": false}],
    "no-eq-null": 2,
    "no-mixed-requires": 0,
    "no-mixed-spaces-and-tabs": 2,
    "no-multiple-empty-lines": [2, {max: 2}],
    "no-trailing-spaces": 2,
    "no-undef": 0,
    "no-underscore-dangle": 0,
    "no-unused-vars": 2,
    "no-var": 2,
    "quotes": [2, "single"],
    "semi": 0,
    "space-before-blocks": [2, "always"],
    "space-before-function-parentheses": [2, "never"],
    "space-return-throw-case": 2,
    "strict": 0
  }
}

$ eslint --ext .js,.jsx test-class.js
$ echo $?

emmenko commented Mar 18, 2015

@nzakas well, disabling classes in the ecmaFeatures configuration doesn't do anything (that's also why I decided to add the rule). Maybe is it a bug?

$ cat test-class.js
class Foo {
  constructor() {
    this.foo = 'bar'
  }
}

$ cat .eslintrc
/* http://eslint.org/docs/rules/ */
{
  "parser": "babel-eslint",
  "ecmaFeatures": {
    "arrowFunctions": true,
    "blockBindings": true,
    "classes": false,
    "defaultParams": true,
    "jsx": true,
    "templateStrings": true
  },
  "env": {
    "browser": true,
    "jasmine": true,
    "node": true
  },
  "rules": {
    "camelcase": 2,
    "consistent-return": 2,
    "curly": [2, "multi"],
    "eol-last": 2,
    "eqeqeq": 2,
    "new-cap": [2, {"capIsNew": false}],
    "no-eq-null": 2,
    "no-mixed-requires": 0,
    "no-mixed-spaces-and-tabs": 2,
    "no-multiple-empty-lines": [2, {max: 2}],
    "no-trailing-spaces": 2,
    "no-undef": 0,
    "no-underscore-dangle": 0,
    "no-unused-vars": 2,
    "no-var": 2,
    "quotes": [2, "single"],
    "semi": 0,
    "space-before-blocks": [2, "always"],
    "space-before-function-parentheses": [2, "never"],
    "space-return-throw-case": 2,
    "strict": 0
  }
}

$ eslint --ext .js,.jsx test-class.js
$ echo $?
@ericelliott

This comment has been minimized.

Show comment
Hide comment
@ericelliott

ericelliott Mar 18, 2015

@nzakas The problems with class and constructors are actually quite well known and quite well documented. There's a whole section on object instantiation in the GoF "Design Patterns" book that exists to get around a catalog of well documented famous OO design problems:

  • Tight coupling - child classes are tightly coupled to the implementation of parent classes, which hurts code reuse.
  • Brittle architecture - because of tight coupling, a change in a base class can ripple out and break many inheriting classes.
  • Duplication by necessity - Every parent/child hierarchy is eventually the "wrong design" for new use cases, but ends up being locked in by existing uses, which forces the creation of largely duplicate, but subtly different classes.
  • Complicated multiple inheritance - Often, you want to inherit from multiple sources. Using class as the primary inheritance mechanism necessitates multiple, often incompatible inheritance paradigms. For an example in the wild, Node's Streams implementation used util.extends for pseudo-classical inheritance. When they went to create duplex streams, they needed to inherit from both readable and writable streams. Since they used util.extends as the mechanism and there can only be one prototype, they chose to inherit from one using util.extends and then inherit from the other by copying properties. So in order to inherit properly from duplex streams, users were forced to do extra work after using util.extends to inherit all the stuff they need from duplex streams, which led to lots of confusion and complaints -- and a proliferation of libraries which exist only to make creation of duplex streams easier. e.g., through.
  • Gorilla Banana Problem - Because class inheritance is not selective (you inherit everything, and the only way not to inherit something is to override it with something else with the same name), "You wanted a banana but what you got was a gorilla holding the banana and the entire jungle." - Joe Armstrong in "Coders at Work"

Google "class considered harmful" for lots of references dating back to before JS was a twinkle in Brendan Eich's eye.

The current class spec is riddled with problems that can prevent the common class -> factory refactor.

That need is common enough that it is included in Martin Fowler's refactoring catalog from the seminal refactoring book.

For more, watch "Classical Inheritance is Obsolete: How to Think in Prototypal OO"

ericelliott commented Mar 18, 2015

@nzakas The problems with class and constructors are actually quite well known and quite well documented. There's a whole section on object instantiation in the GoF "Design Patterns" book that exists to get around a catalog of well documented famous OO design problems:

  • Tight coupling - child classes are tightly coupled to the implementation of parent classes, which hurts code reuse.
  • Brittle architecture - because of tight coupling, a change in a base class can ripple out and break many inheriting classes.
  • Duplication by necessity - Every parent/child hierarchy is eventually the "wrong design" for new use cases, but ends up being locked in by existing uses, which forces the creation of largely duplicate, but subtly different classes.
  • Complicated multiple inheritance - Often, you want to inherit from multiple sources. Using class as the primary inheritance mechanism necessitates multiple, often incompatible inheritance paradigms. For an example in the wild, Node's Streams implementation used util.extends for pseudo-classical inheritance. When they went to create duplex streams, they needed to inherit from both readable and writable streams. Since they used util.extends as the mechanism and there can only be one prototype, they chose to inherit from one using util.extends and then inherit from the other by copying properties. So in order to inherit properly from duplex streams, users were forced to do extra work after using util.extends to inherit all the stuff they need from duplex streams, which led to lots of confusion and complaints -- and a proliferation of libraries which exist only to make creation of duplex streams easier. e.g., through.
  • Gorilla Banana Problem - Because class inheritance is not selective (you inherit everything, and the only way not to inherit something is to override it with something else with the same name), "You wanted a banana but what you got was a gorilla holding the banana and the entire jungle." - Joe Armstrong in "Coders at Work"

Google "class considered harmful" for lots of references dating back to before JS was a twinkle in Brendan Eich's eye.

The current class spec is riddled with problems that can prevent the common class -> factory refactor.

That need is common enough that it is included in Martin Fowler's refactoring catalog from the seminal refactoring book.

For more, watch "Classical Inheritance is Obsolete: How to Think in Prototypal OO"

@emmenko

This comment has been minimized.

Show comment
Hide comment
@emmenko

emmenko Mar 18, 2015

@ericelliott 👍

@nzakas as I said here #2097 (comment) it may look like a babel-eslint problem. So I guess the recommended way to do that would be to simply disable classes ecmaFeatures: { classes: false } right?
If so then we don't need this rule at all. Sorry then for the misunderstanding.

emmenko commented Mar 18, 2015

@ericelliott 👍

@nzakas as I said here #2097 (comment) it may look like a babel-eslint problem. So I guess the recommended way to do that would be to simply disable classes ecmaFeatures: { classes: false } right?
If so then we don't need this rule at all. Sorry then for the misunderstanding.

@emmenko

This comment has been minimized.

Show comment
Hide comment
@emmenko

emmenko Mar 18, 2015

So in the end babel-eslint simply ignores ecmaFeatures flags, so I have to use a plugin then. I'm creating one (for who's interested)

https://github.com/emmenko/eslint-plugin-no-class

emmenko commented Mar 18, 2015

So in the end babel-eslint simply ignores ecmaFeatures flags, so I have to use a plugin then. I'm creating one (for who's interested)

https://github.com/emmenko/eslint-plugin-no-class

@emmenko emmenko closed this Mar 18, 2015

@emmenko emmenko deleted the emmenko:no-class-rule branch Mar 18, 2015

@robertpenner

This comment has been minimized.

Show comment
Hide comment
@robertpenner

robertpenner Mar 18, 2015

If your gripe is with inheritance, then ban extends. In practice, most occurrences of class will not involve inheritance.

robertpenner commented Mar 18, 2015

If your gripe is with inheritance, then ban extends. In practice, most occurrences of class will not involve inheritance.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 18, 2015

Member

@ericelliott I read your article and while I understand your point of view, I don't share it. The bar for core ESLint rules is relatively high, and it's far too early to declare anything in ES6 as "bad".

The beauty of ESLint, though, is that you can always create and share your own rules to further any agenda that you want. We made it this way so we don't all have to agree in order for ESLint to be useful.

Member

nzakas commented Mar 18, 2015

@ericelliott I read your article and while I understand your point of view, I don't share it. The bar for core ESLint rules is relatively high, and it's far too early to declare anything in ES6 as "bad".

The beauty of ESLint, though, is that you can always create and share your own rules to further any agenda that you want. We made it this way so we don't all have to agree in order for ESLint to be useful.

@eslint eslint locked and limited conversation to collaborators Mar 18, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.