Skip to content
This repository has been archived by the owner on Jan 19, 2019. It is now read-only.

add a recommended preset #144

Closed
aladdin-add opened this issue Jul 14, 2018 · 27 comments
Closed

add a recommended preset #144

aladdin-add opened this issue Jul 14, 2018 · 27 comments

Comments

@aladdin-add
Copy link

No description provided.

@benjie
Copy link

benjie commented Jul 22, 2018

Can anyone share the typescript rules they're using? Do you just copy/paste from the README and set them all to 'error'?

@nickmccurdy
Copy link

I'm also unclear about usage. Should all of these rules be enabled and then have select core ESLint rules to enable, or do these replace them (except for no-unused-vars)?

@jorgegonzalez
Copy link

jorgegonzalez commented Jul 25, 2018

We're using these in Zoe.

@ChristianStornowski
Copy link

ChristianStornowski commented Oct 15, 2018

Im using this tslint recommendation aligned configuration:

    "typescript/adjacent-overload-signatures": "error",
    "typescript/class-name-casing": "error",
    "typescript/interface-name-prefix": [
      "error",
      "always"
    ],
    "typescript/no-type-alias": "error",
    "typescript/explicit-member-accessibility": "error",
    "typescript/member-ordering": "error",
    "typescript/no-angle-bracket-type-assertion": "error",
    "typescript/no-empty-interface": "error",
    "typescript/prefer-namespace-keyword": "error",
    "typescript/no-namespace": "error",
    "typescript/no-parameter-properties": "error",
    "typescript/no-triple-slash-reference": "error",
    "typescript/no-var-requires": "error",

@ChristianStornowski
Copy link

I did some homework:

https://www.npmjs.com/package/eslint-config-typescript-recommended

Maybe it could be integrated into this plugin. :-)

@j-f1
Copy link
Collaborator

j-f1 commented Nov 25, 2018

@ChristianStornowski a quick note: valid-jsdoc is different from jsdoc-format because it checks for content of the JSDoc instead of just syntax.

@ChristianStornowski
Copy link

ChristianStornowski commented Nov 26, 2018

Thanks for this hint. :-)

If i'm reading this correct: https://eslint.org/docs/rules/valid-jsdoc#options

i only have to set requireReturn to false and only (except constructors) syntax check is enabled.

@j-f1
Copy link
Collaborator

j-f1 commented Nov 26, 2018

@ChristianStornowski Unfortunately, it will still error if a value is explicitly returned.

@weirdpattern
Copy link
Collaborator

I'm the owner of eslint-config-typescript and my intention was (and still is) to complement this project with that one... I'm willing to work with anyone that is interested in making that happen 😄

@ChristianStornowski
Copy link

They are different projects in the npm world:

https://www.npmjs.com/package/eslint-config-standard-with-typescript
https://www.npmjs.com/package/eslint-config-typescript
https://www.npmjs.com/package/eslint-config-typescript-recommended

we should combine them together.

The question is who should be the owner. Should be eslint-plugin-typescript the owner for this configs?

@mightyiam
Copy link
Contributor

Re: combining them together

I am the person behind https://www.npmjs.com/package/eslint-config-standard-with-typescript.

I don't see a good reason for combining them together.

Further, I feel that recommendations should be outside the scope of this project.

Let the community provide shareable configs, is how I see it. I would close this issue.

@benjie
Copy link

benjie commented Nov 27, 2018

Definitely add them to the README though 👍

@bradzacher bradzacher added this to the 1.0.0 milestone Nov 27, 2018
@bradzacher
Copy link
Owner

It's common practice for plugin authors to provide at the very least a "recommended" config set, if not more than that [1] [2] [3] [4]

The idea is to ease adoption of the plugin: i.e. "I like the idea of linting [react/typescript/jest/etc], but I don't want to read the docs and configure each rule myself, I'll start with the recommended and then tweak based on what I love/hate".

We'll definitely be looking to add at least a recommended config; we'll review the community (i.e. all of your) configs and figure out what is the best recommendation.

Aim will be to have this in for the 1.0.0 release.

@bradzacher
Copy link
Owner

bradzacher commented Dec 17, 2018

(copying comment from #204 (comment) for visibility)


I went through 4 configs that I know with config for our plugin, and came up with the following recommendations by merging them together.

Please provide feedback.

The configs for reference, with keys sorted:

eslint-config-typescript-recommended

https://github.com/diva-e/eslint-config-typescript-recommended/blob/master/index.js#L124-L140

{
    "typescript/adjacent-overload-signatures": "error",
    "typescript/class-name-casing": "error",
    "typescript/explicit-member-accessibility": "error",
    "typescript/interface-name-prefix": ["error", "always"],
    "typescript/member-ordering": "error",
    "typescript/no-angle-bracket-type-assertion": "error",
    "typescript/no-empty-interface": "error",
    "typescript/no-explicit-any": "off",
    "typescript/no-namespace": "error",
    "typescript/no-parameter-properties": "off",
    "typescript/no-triple-slash-reference": "error",
    "typescript/no-type-alias": "error",
    "typescript/no-var-requires": "error",
    "typescript/prefer-namespace-keyword": "error"
}

eslint-config-typescript

https://github.com/weirdpattern/eslint-config-typescript/blob/master/src/typescript.js#L21-L68

{
    "typescript/adjacent-overload-signatures": "error",
    "typescript/class-name-casing": "error",
    "typescript/explicit-function-return-type": [
        "error",
        {
            "allowExpressions": true
        }
    ],
    "typescript/explicit-member-accessibility": "error",
    "typescript/interface-name-prefix": "error",
    "typescript/member-delimiter-style": "error",
    "typescript/member-ordering": [
        "error",
        {
            "default": [
                "static-field",
                "private-field",
                "protected-field",
                "public-field",
                "constructor",
                "public-method",
                "protected-method",
                "private-method"
            ]
        }
    ],
    "typescript/no-angle-bracket-type-assertion": "error",
    "typescript/no-array-constructor": "error",
    "typescript/no-inferrable-types": [
        "error",
        {
            "ignoreProperties": true,
            "ignoreParameters": true
        }
    ],
    "typescript/no-namespace": [
        "error",
        {
            "allowDeclarations": true
        }
    ],
    "typescript/no-non-null-assertion": "error",
    "typescript/no-parameter-properties": "error",
    "typescript/no-triple-slash-reference": "error",
    "typescript/no-use-before-define": "error",
    "typescript/no-var-requires": "error",
    "typescript/prefer-namespace-keyword": "error",
    "typescript/type-annotation-spacing": "error"
}

eslint-config-standard-with-typescript

https://github.com/mightyiam/eslint-config-standard-with-typescript/blob/master/src/index.ts#L19-L32

{
    "typescript/adjacent-overload-signatures": "error",
    "typescript/explicit-function-return-type": "error",
    "typescript/explicit-member-accessibility": "error",
    "typescript/member-delimiter-style": [
        "error",
        {
            "delimiter": "none"
        }
    ],
    "typescript/no-angle-bracket-type-assertion": "error",
    "typescript/no-array-constructor": "error",
    "typescript/no-empty-interface": "error",
    "typescript/no-namespace": "error",
    "typescript/no-non-null-assertion": "error",
    "typescript/no-triple-slash-reference": "error",
    "typescript/no-unused-vars": "error",
    "typescript/no-use-before-define": [
        "error",
        {
            "functions": false,
            "classes": false,
            "variables": false,
            "typedefs": false
        }
    ],
    "typescript/no-var-requires": "error",
    "typescript/type-annotation-spacing": "error"
}

eslint-config-assignar

This is the last company I worked at, and I did the eslint config :)
https://github.com/assignar/eslint-config-assignar/blob/develop/packages/eslint-config-assignar-base/src/plugins/typescript.ts

{
    "typescript/adjacent-overload-signatures": "error",
    "typescript/class-name-casing": "error",
    "typescript/explicit-function-return-type": "off",
    "typescript/explicit-member-accessibility": "error",
    "typescript/interface-name-prefix": ["warn", "never"],
    "typescript/member-delimiter-style": [
        "error",
        {
            "delimiter": "none",
            "requireLast": true,
            "ignoreSingleLine": true
        }
    ],
    "typescript/member-naming": "off",
    "typescript/member-ordering": "off",
    "typescript/no-angle-bracket-type-assertion": "error",
    "typescript/no-array-constructor": "error",
    "typescript/no-empty-interface": "off",
    "typescript/no-explicit-any": "warn",
    "typescript/no-inferrable-types": "off",
    "typescript/no-namespace": [
        "error",
        {
            "allowDeclarations": false,
            "allowDefinitionFiles": true
        }
    ],
    "typescript/no-non-null-assertion": "warn",
    "typescript/no-parameter-properties": "error",
    "typescript/no-triple-slash-reference": "warn",
    "typescript/no-type-alias": "off",
    "typescript/no-unused-vars": "error",
    "typescript/no-use-before-define": "error",
    "typescript/no-var-requires": "off",
    "typescript/prefer-namespace-keyword": "off",
    "typescript/type-annotation-spacing": [
        "error",
        {
            "before": false,
            "after": true,
            "overrides": {
                "arrow": {
                    "before": true,
                    "after": true
                },
                "colon": {
                    "before": false,
                    "after": true
                }
            }
        }
    ]
}

Recommended Config

{
    "typescript/adjacent-overload-signatures": "error",
    "typescript/array-type": ["error", "array"],
    "typescript/ban-types": [
        "error",
        {
            "types": {
                "Boolean": {
                    "message": "Use boolean instead",
                    "fixWith": "boolean"
                },
                "Number": {
                    "message": "Use number instead",
                    "fixWith": "number"
                },
                "Object": {
                    "message": "Use Record<string, any> instead",
                    "fixWith": "Record<string, any>"
                },
                "Symbol": {
                    "message": "Use symbol instead",
                    "fixWith": "symbol"
                }
            }
        }
    ],
    "typescript/camelcase": [
        "error",
        {
            "allow": ["^UNSAFE_"],
            "ignoreDestructuring": false,
            "properties": "never"
        }
    ],
    "typescript/class-name-casing": "error",
    "typescript/explicit-function-return-type": [
        "error",
        {
            "allowExpressions": true
        }
    ],
    "typescript/explicit-member-accessibility": "error",
    "typescript/generic-type-naming": ["error", "'^T[A-Z][a-zA-Z]+$'"],
    "indent": "off",
    "typescript/indent": ["error", 4],
    "typescript/interface-name-prefix": ["error", "never"],
    "typescript/member-delimiter-style": "error",
    "typescript/member-ordering": "error",
    "typescript/no-angle-bracket-type-assertion": "error",
    "typescript/no-array-constructor": "error",
    "typescript/no-empty-interface": "error",
    "typescript/no-explicit-any": "error",
    "typescript/no-inferrable-types": [
        "error",
        {
            "ignoreProperties": true,
            "ignoreParameters": true
        }
    ],
    "typescript/no-misused-new": "error",
    "typescript/no-namespace": [
        "error",
        {
            "allowDeclarations": true
        }
    ],
    "typescript/no-non-null-assertion": "error",
    "typescript/no-object-literal-type-assertion": "error",
    "typescript/no-parameter-properties": "error",
    "typescript/no-triple-slash-reference": "error",
    "typescript/no-type-alias": "off",
    "no-unused-vars": "off",
    "typescript/no-unused-vars": "off",
    "typescript/no-use-before-define": "error",
    "typescript/no-var-requires": "error",
    "typescript/prefer-interface": "error",
    "typescript/prefer-namespace-keyword": "error",
    "typescript/type-annotation-spacing": "error"
}

@corbinu
Copy link
Contributor

corbinu commented Dec 17, 2018

@bradzacher is typescript/no-unused-vars supposed to be off?

Just a few thoughts that are totally my personal opinion

  • typescript/explicit-function-return-type - I would turn off but I still think it should be recommended
  • typescript/no-explicit-any - I would turn off but I still think it should be recommended
  • typescript/no-angle-bracket-type-assertion - This is only for those using JSX the other syntax is still used a lot in libs unrelated to React
  • typescript/generic-type-naming - This regex rejects just a straight T which seems a little odd to me but would love to hear others thoughts

@j-f1
Copy link
Collaborator

j-f1 commented Dec 17, 2018

@corbinu For the angle bracket type assertion, I think the as syntax is much easier to read.

@corbinu
Copy link
Contributor

corbinu commented Dec 17, 2018

@j-f1 guess maybe it just looks natural to me as Java was the second language I learned so casting just looks like that to me ;) and also because I started in TypeScript so early when this was the only option.

Either way I do think that the recommended settings should favor conforming to how typescript programmers currently format their code rather than be prescriptive as that will most likely hurt adoption. A lot of large old TypeScript projects still use the bracket format. Plus if they want a more prescriptive format that is kinda what Prettier is for in my mind.

@armano2
Copy link
Contributor

armano2 commented Dec 17, 2018

recomended configuration should be default one set in rule, that user is not forced to setup configuration to make it work, for example:

"typescript/no-namespace": [ "error", {
  "allowDeclarations": true
}],

allowDeclarations should be set by default to true

@j-f1 j-f1 pinned this issue Dec 17, 2018
@j-f1
Copy link
Collaborator

j-f1 commented Dec 17, 2018

Plus if they want a more prescriptive format that is kinda what Prettier is for in my mind.

Note that Prettier doesn’t prescribe a style here — its goal is to print the syntax as-is without changing it.

@corbinu
Copy link
Contributor

corbinu commented Dec 17, 2018

@j-f1 guess we will have to disagree here I am basing this on prettier's "An opinionated code formatter" from their docs. I feel like that is not the goal of prettier even if it is not the current behavior of prettier

@bradzacher
Copy link
Owner

@corbinu

typescript/explicit-function-return-type - I would turn off but I still think it should be recommended

Probably best for us to turn it on as a warning then?
I'm not sure what to do with these "best practice" types of rules. Do we turn them on because they're best practices that aren't followed a lot, or do we turn them off because they're hard for people to get used to?

typescript/no-explicit-any - I would turn off but I still think it should be recommended

Being an experienced TS dev; I personally prefer it on, but on as a warning. If only so that other devs on my team are less likely to use any 😄.
One of the biggest problems I've seen with new TS devs is that they often throw their hands up in the air and drop an any when typing gets a little hard (a side effect of devs trying to write TS as if it was JS).

I'm of the opinion that using any is a code-smell, and we should recommend you don't do it.

typescript/no-angle-bracket-type-assertion - This is only for those using JSX the other syntax is still used a lot in libs unrelated to React

From what I've seen in the docs that the TS team releases, using as is the new standard. Using angle brackets causes confusion with JSX syntax and is ultimately ugly syntax.

I came from a java/c++/c# background, but I much prefer the as syntax anyways; much easier to follow.
Definitely prefer it to flow's casting as well const x = ('asdf': MyType)

typescript/generic-type-naming - This regex rejects just a straight T which seems a little odd to me but would love to hear others thoughts

I just copy pasted the regex. But in an ideal world you should probably name your generics with more than just T. We can defs change it to allow just the single T though, considering that's what most people do with a single generic.
(hey, maybe we should add this as a config option?)

I'm not sure if that should be there though... is that considered an anti-pattern in TS now? (similar to how IInterface is an antipattern now).


@armano2 Yeah we'll figure out what our defaults are with this recommended set, and we'll make the changes across the board to match.

@ChristianStornowski
Copy link

Maybe we should considering palantir/tslint#1142. There are a lot of discussions and implementation https://github.com/palantir/tslint/commits/master to be more aligned with https://www.npmjs.com/package/tslint-microsoft-contrib.

@corbinu
Copy link
Contributor

corbinu commented Dec 17, 2018

@bradzacher Thanks for the feedback makes sense as for the "best practice" types of rules I have seen a lot of eslint packages have both an "essential" config that doesn't include them and then a "recommended" that does.

@bradzacher
Copy link
Owner

See #261

bradzacher added a commit that referenced this issue Dec 22, 2018
Fixes #144
Requires ~~#259~~, ~~#260~~.

- added a util to make it standardised and easier to add default config for a rule
- configured recommended based on #144
  - purposely switched `recommended` prop to be `"error" | "warning" | false`
    - inside the eslint repo, it should be `true`. otherwise it's just a property that isn't used officially by eslint. It's truthy so `eslint-docs` still work.
  - changed recommended generator to accept `"error"`/`"warning"` for more configurability.
- adjusted default config of certain rules that didn't match our recommendations.
@bradzacher bradzacher unpinned this issue Dec 22, 2018
@corbinu
Copy link
Contributor

corbinu commented Dec 27, 2018

@bradzacher I have started using this however I am getting errors that the recommended config sets some rules as "warning" instead of "warn" for example "typescript/explicit-function-return-type"

@j-f1
Copy link
Collaborator

j-f1 commented Dec 27, 2018

#263 @corbinu

@corbinu
Copy link
Contributor

corbinu commented Dec 29, 2018

@j-f1 Thanks!

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

No branches or pull requests