feat(eslint-plugin)!: [array-type] add "default", "readonly" options

[a-tarasyuk]

BREAKING CHANGE: changes config structure

Fixes #635

[bradzacher]

The next release will be a breaking change, so let's rework the options completely to make them better.

type ArrayOption = 'array' | 'generic' | 'array-simple';
type Options = [
  {
    // default case for all arrays
    default: ArrayOption,
    // optional override for readonly arrays
    readonly?: ArrayOption,
  },
];

const defaultOptions = [
  {
    default: 'array',
  },
]

This way the user can't provide some weird state of ['generic', { ReadonlyArray: true }.

[a-tarasyuk]

@bradzacher

type Options = [
  {
    // default case for all arrays
    default: ArrayOption,
    // optional override for readonly arrays
    readonly?: ArrayOption,
  },
];

The new option definition should behave like so

// "array-type": ["error", { "default": "array" }]
const x: ReadonlyArray<number> = [1];
const x: Array<number> = [1];
// fixes to
const x: ReadonlyArray<number> = [1];
const x: number[] = [1];

// "array-type": ["error", { "default": "array", "readonly": "array" }]
const x: ReadonlyArray<number> = [1];
const y: Array<number> = [1];
// fixes to
const x: readonly number[] = [1];
const y: number[] = [1];

// "array-type": ["error", { "default": "generic" }]
const a: string[] = [];
const x: readonly number[][] = [];
// fixes to
const a: Array<string> = [];
const x: readonly number[][] = [];

// "array-type": ["error", { "default": "generic", "readonly": "generic" }]
const a: string[] = [];
const x: readonly number[][] = [];
// fixes to
const a: Array<string> = [];
const x: ReadonlyArray<Array<number>> = [];

Is it right?

[bradzacher]

Almost!

Sorry, I didn't explain my intention clearly enough:
The default config should be this {default:'array', readonly: options.default}, i.e. the readonly option mirrors the default option unless explicitly provided.

So your examples were almost right.
The new option definition should behave like so

 // "array-type": ["error", { "default": "array" }]
 const x: ReadonlyArray<number> = [1];
 const x: Array<number> = [1];
 // fixes to
-const x: ReadonlyArray<number> = [1];
+const x: readonly number[] = [1];
 const x: number[] = [1];

// "array-type": ["error", { "default": "array", "readonly": "array" }]
const x: ReadonlyArray<number> = [1];
const y: Array<number> = [1];
// fixes to
const x: readonly number[] = [1];
const y: number[] = [1];

 // "array-type": ["error", { "default": "generic" }]
 const a: string[] = [];
 const x: readonly number[][] = [];
 // fixes to
 const a: Array<string> = [];
-const x: readonly number[][] = [];
+const x: ReadonlyArray<Array<number>> = [];

// "array-type": ["error", { "default": "generic", "readonly": "generic" }]
const a: string[] = [];
const x: readonly number[][] = [];
// fixes to
const a: Array<string> = [];
const x: ReadonlyArray<Array<number>> = [];

And of course the situations where the two differ:

// "array-type": ["error", { "default": "array", "readonly": "generic" }]
const a: string[] = [];
const x: readonly number[][] = [];
// fixes to
const a: string[] = [];
const x: ReadonlyArray<number[]> = [];

// "array-type": ["error", { "default": "generic", "readonly": "array" }]
const a: string[] = [];
const x: readonly number[][] = [];
// fixes to
const a: Array<string> = [];
const x: readonly Array<number>[] = [];

[a-tarasyuk]

@bradzacher Which option can skip check for ReadonlyArray?

[bradzacher]

I'm not sure I quite understand - do you mean you were looking for an option to ignore validating readonly arrays completely? I don't think that's a valid state for this rule, as you then can have inconsistent definitions.

{ "default": "array" } will enforce that all your array types use (readonly)? foo[].
{ "default": "generic" } will enforce that all your arrays use (Readonly)?Array<foo>.
{ "default": "array", "readonly": "generic" } will enforce that all mutable arrays use foo[] and your readonly arrays use ReadonlyArray<foo>.
{ "default": "generic", "readonly": "array" } will enforce that all mutable arrays use Array<foo> and your readonly arrays use readonly foo[].

The "array-simple" option will interop similarly, but I cbf enumerating those examples :)

[a-tarasyuk]

@bradzacher

Do you mean you were looking for an option to ignore validating readonly arrays completely? I don't think that's a valid state for this rule, as you then can have inconsistent definitions.

It was the main idea to add an option to "array", to have the opportunity to ignore ReadonlyArray. Right now users cannot have the behavior like in tslint:array-type: ["array"], "array" option behaves differently in typescript-eslint.

[bradzacher]

I think you've misunderstood me.
{ "default": "array", "readonly": "generic" } will enforce that all mutable arrays use foo[] and your readonly arrays use ReadonlyArray<foo>.

Which is almost exactly the same as tslints array setting.

The difference being that tslint will let you use readonly foo[], where as this configuration will enforce that you instead use ReadonlyArray<foo>

[bradzacher]

just need to fix two of your tests.
everything else LGTM, great work.

[bradzacher]

LGTM!

[codecov]

Codecov Report

Merging #654 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #654      +/-   ##
==========================================
+ Coverage   94.45%   94.46%   +0.01%     
==========================================
  Files         113      113              
  Lines        4705     4719      +14     
  Branches     1288     1297       +9     
==========================================
+ Hits         4444     4458      +14     
  Misses        150      150              
  Partials      111      111

Impacted Files	Coverage Δ
packages/eslint-plugin/src/rules/array-type.ts	92.63% <100%> (+1.27%)	⬆️

[octogonz]

@bradzacher I noticed that the default for this rule is to require readonly string[] instead of ReadonlyArray<string>. This is a bleeding edge syntax that produces a syntax error for TypeScript versions prior to 3.4. When I enabled this rule and ran the fixer, we received reports that our NPM package was no longer usable with older releases of TypeScript.

By contrast, requiring string[] instead of Array<string> seems idiomatic and perfectly fine. Therefore, I wonder if the default should be like this instead:

"@typescript-eslint/array-type": [
  "error",
  {
    default: "array",
    readonly: "generic"
  }
],

What do you think?

[bradzacher]

I don't have a problem with recommending the bleeding edge, as the behaviour is documented in the readme.

I think it's better to recommend the most consistent syntax, and have people configure the support for their environment if they don't have the new syntax. Yours is the first complaint about it, so it might be uncommon to not be on 3.4? I don't know exactly.

Regardless, changing the default option is a breaking change, which means it'd have to wait for a while if we did want to make the change.

[octogonz]

If we measure number of projects, it's probably very uncommon to not be using the latest bleeding edge. Statistically the vast majority of projects are tiny and probably just got started. Whereas if we measure number of dollars earned, older compilers are probably way more likely: Upgrading TypeScript can require nontrivial effort for a production code base. Besides all the files that need fixes, it may also require upgrading @types packages, which may require upgrading the underlying library. I worked on a project one time where two people worked for two full weeks to get everything to compile again, and it wasn't even a major version increment for the compiler.