fix: deep copy standardRules config

[Patrick-Remy]

This fixes #303 invalid configuration for dot-notation

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x ] Bug fix
[ ] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)
Instead of assigning the equivalents config object, the must be deep copied

Which issue (if any) does this pull request address?
Addresses #303

Is there anything you'd like reviewers to focus on?
more elegant way for [...val] as unknown as T

[LinusU]

Awesome! Thank you for finding the underlying cause of the issue, this is hilarious 😁

What do you think about this approach for fixing it instead though:

diff --git a/src/index.ts b/src/index.ts
index a6254b5..05faf51 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -26,6 +26,10 @@ function fromEntries<T> (iterable: Array<[string, T]>): { [key: string]: T } {
   }, {})
 }
 
+function clone<T extends object | string | number | boolean | null> (value: T): T {
+  return JSON.parse(JSON.stringify(value))
+}
+
 export = {
   extends: 'eslint-config-standard',
   plugins: ['@typescript-eslint'],
@@ -44,7 +48,7 @@ export = {
         'no-use-before-define': 'off',
 
         // @typescript-eslint versions of Standard.js rules:
-        ...fromEntries(equivalents.map((name) => [`@typescript-eslint/${name}`, standardRules[name]])),
+        ...fromEntries(equivalents.map((name) => [`@typescript-eslint/${name}`, clone(standardRules[name])])),
         '@typescript-eslint/no-use-before-define': ['error', {
           functions: false,
           classes: false,

The benefits are:

1. It actually does a deep clone, your code currently does a shallow clone
2. It keeps the behaviour of fromEntries aligned with the official Object.fromEntries (which we can switch to when targeting newer Node.js version)
3. Avoids cloning the newly created [name, 'off'] array

---

sidebar: T extends object | string | number | boolean | null isn't actually 100% accurate, but for this case I think it's good enough. See more discussion here: microsoft/TypeScript#1897

[Patrick-Remy]

Also just thought about that. I updated the commit 👍

[mightyiam]

Thank you for this PR, @Patrick-Remy. I would like to merge this fix, but:

1. There are some extraneous files included.
2. I would like the pull request to introduce a test that prevents this from regressing.

[Patrick-Remy]

Yes, sorry for the history files. I added an extensive test for it 👍

[mightyiam]

Thank you for the test, @Patrick-Remy. It covers what I am concerned about, but I feel it covers too much and is slightly more complex than I feel comfortable with. I'd like to ask you to make the following changes, if you don't mind.

[mightyiam]

No need for this type, then, I guess.

[Patrick-Remy]

It is required due to Element implicitly has an 'any' type because expression of type 'string' can't be used to index type if use standardRules[ruleName].

[Patrick-Remy]

I think the tests are both required.

The iteration is required, as the error can occur, if there are any references, also from different keys. You can reproduce it by following these steps:

1. Create a new project with this package.json

{
  "dependencies": {
    "@types/node": "^14.0.5",
    "@typescript-eslint/eslint-plugin": "^3.0.0",
    "@typescript-eslint/parser": "^3.0.0",
    "eslint": "^7.1.0",
    "eslint-config-standard": "^14.1.1",
    "eslint-plugin-import": "^2.20.2",
    "eslint-plugin-node": "^11.1.0",
    "eslint-plugin-promise": "^4.2.1",
    "eslint-plugin-standard": "^4.0.1",
    "typescript": "^3.9.3"
  }
}

this tsconfig.json

{
  "compilerOptions": {
    "target": "es2018",
    "baseUrl": "src",
    "outDir": "lib",
    "module": "commonjs"
  },
  "include": [
    "src"
  ]
}

and this .eslintrc.js

const eslintrc_json_1 = require("eslint-config-standard/eslintrc.json");
module.exports = {
  extends: 'eslint-config-standard',
  plugins: ['@typescript-eslint'],
  overrides: [
    {
      files: ['*.ts', '*.tsx'],
      parser: '@typescript-eslint/parser',
      rules: {
        '@typescript-eslint/dot-notation': eslintrc_json_1.rules['arrow-spacing']
      }
    }
  ]
}

2. Run npm install
3. Now edit node_modules/eslint-config-standard/eslintrc.json and replace the rule config for arrow-spacing with ["error", { "allowKeywords": true }].
4. Open eslint-test/node_modules/eslint/lib/rules/arrow-spacing.js and add

allowKeywords: {
    type: 'boolean',
    default: true,
},
allowPattern: {
    type: 'string',
    default: '',
}

to the schema properties.
5. Now run npx eslint src and see this error

Error: .eslintrc.js#overrides[0]:
        Configuration for rule "@typescript-eslint/dot-notation" is invalid:
        Value {"allowKeywords":true,"before":true,"after":true,"allowPattern":"","allowPrivateClassPropertyAccess":false} should NOT have additional properties.

So before and after default properties have been applied to the arrow-spacing config object, and therefor also to the @typescript-eslint/dot-notation rule.
I think the problem comes from default values and eslint's schema, but haven't got as deep in eslint's code. I also tested it with different rules where config schema is already the same, but without success. Therefor the above modification of the schema is required, because no other package has the same schema as @typescript-eslint/dot-notation.

---

References inbetween the override could also be problematic. Just imagine my personal config extends eslint-config-standard-with-typescript, and imports it to modificate it like

const config = require('eslint-config-standard-with-typescript')

config.overrides[0].rules['some-rule'] = ['error', {
  custom: false
}]

module.exports = config

If this lib would use the same config object for 'some-rule' and 'some-different-rule', this will result in unexpectedly modifying both of them. So it would be a bad style to use references inside the config.

Hope you can get my points.

[mightyiam]

@Patrick-Remy the only reuse-of-rule-by-reference we have is same-key from eslint-config-standard. That is removed in this PR and that is the only thing that should be tested against in this PR. More comprehensive testing is not worth the slightly increased complexity of the test, because there is no reasonable expectation that what it covers will ever be actually hit. Please do make those changes so that we could make a release.

[Patrick-Remy]

In my opinion tests should not use knowledge about the implementation details, to only cover that problems never reoccur in a specific case. The test now isn't preventing the root cause in a general way any more. It also hadn't any big performance downsides and it's now possible to pass the test, but run into this error again in a different case. But possibly that's only about philosophy.

However, I also think a fast release is important now, as the package is broken for the most. If there are any changes required, please let me now.

[mightyiam]

Thank you, @Patrick-Remy 💙