search

mdx-js / eslint-mdx

ESLint Parser/Plugin for MDX
https://npmjs.org/eslint-plugin-mdx
MIT License
264 stars 30 forks source link

Regression of stable/reliable run since v1.12 #339

Closed codejedi365 closed 3 years ago

codejedi365 commented 3 years ago

Subject of the issue

Semver Minor upgrades have made functionality & performance worse over the last four versions. Yesterday, I went to run the linter after trashing package-lock.json and re-installing the updates, I found npm run lint hitting a runtime error. I incrementally rolled back the updates and found none of the latest versions are stable or working well.

In summary I found:

I isolated the issue to be this plugin via .eslintrc.js as I commented out the plugin:mdx/recommended and everything works if it doesn't pass through this plugin.

Your environment

Steps to reproduce

# 0. configure pre-req of NodeJs v12 & v14.  Use NVM to swap versions quickly.
# 1. Clone my repo
$ git clone https://github.com/codejedi365/treemap-js.git

# 2. Checkout commit
$ git checkout -b lint-test bd37e75f

# 3. Activate node version & upgrade npm (will select version based on .nvmrc)
$ nvm use
$ nvm install-latest-npm

# 4. Install dependencies & verify latest version is installed (v1.15.0)
$ npm install
$ npm ls eslint-plugin-mdx
treemap@1.0.2
└── eslint-plugin-mdx@1.15.0

# 5. Run lint command
$ npm run lint

# 6. Roll-back version of eslint-plugin-mdx
$ npm i -D eslint-plugin-mdx@1.14.0

# 7. Repeat steps 5 & 6 incrementally for v1.13.0, v1.12.0, v1.11.0 and see behavior below.

Expected behaviour

What should happen? eslint should search through the entire project directory for the specified file extensions and run the linting rules against the specific file type. It should parse markdown files with the mdx plugin and identify flaws. The other files will be parsed by their code specific parsers. The mdx plugin should also evaluate the code segments within a markdown file and apply code style rules to those blocks as well. This should not take a long time for the amount of files I have. The CI Pipeline results returned in 7sec and even less on my desktop machine in v1.11.0

Valid CI pipeline results are available for review at https://github.com/codejedi365/treemap-js/runs/3407176892?check_suite_focus=true.

Actual behaviour

In v1.14.0 & v1.15.0:

$ npm exec eslint . --ext ts,js,json,md,mdx,.remarkrc

Oops! Something went wrong! :(

ESLint: 7.32.0

TypeError: Cannot read property 'indent' of undefined
    at adjustMessage (/Users/codejedi365/Development/workbench/workspace/TreeMapUtil/node_modules/eslint-plugin-markdown/lib/processor.js:333:65)
    at Array.map (<anonymous>)
    at /Users/codejedi365/Development/workbench/workspace/TreeMapUtil/node_modules/eslint-plugin-markdown/lib/processor.js:390:22
    at Array.map (<anonymous>)
    at Object.postprocess (/Users/codejedi365/Development/workbench/workspace/TreeMapUtil/node_modules/eslint-plugin-markdown/lib/processor.js:387:34)
    at postprocess (/Users/codejedi365/Development/workbench/workspace/TreeMapUtil/node_modules/eslint-plugin-mdx/lib/processors/remark.js:28:61)
    at Linter._verifyWithProcessor (/Users/codejedi365/Development/workbench/workspace/TreeMapUtil/node_modules/eslint/lib/linter/linter.js:1339:16)
    at Linter._verifyWithConfigArray (/Users/codejedi365/Development/workbench/workspace/TreeMapUtil/node_modules/eslint/lib/linter/linter.js:1273:25)
    at Linter.verify (/Users/codejedi365/Development/workbench/workspace/TreeMapUtil/node_modules/eslint/lib/linter/linter.js:1235:25)
    at Linter.ESLinter.verify (/Users/codejedi365/Development/workbench/workspace/TreeMapUtil/node_modules/eslint-plugin-mdx/lib/processors/options.js:33:19)

In v1.13.0, it never returns >2min.

$ npm exec eslint . --ext ts,js,json,md,mdx,.remarkrc

^C

In v1.12.0: It resolves but it will print out all the issues, including the summary, and then just hang for 30 seconds without doing anything.

$ npm exec eslint . --ext ts,js,json,md,mdx,.remarkrc

.../README.md
  35:25  error  Unable to resolve path to module 'treemap'
                                                                                                                                             import/no-unresolved
  35:34  error  Insert `;`
                                                                                                                                             prettier/prettier
   0:0   error  `processSync` finished async. Use `process` instead
                                                                                                                                             null-null
  30:1   error  Insert `⏎`
                                                                                                                                             prettier/prettier
  35:34  error  Insert `;`
                                                                                                                                             prettier/prettier

.../package.json
  1:1  error  JSON is not sorted  JSON sorting

/Users/codejedi365/Development/workbench/workspace/TreeMapUtil/CHANGELOG.md
  0:0  error  `processSync` finished async. Use `process` instead
                                                                                                                                             null-null

/Users/codejedi365/Development/workbench/workspace/TreeMapUtil/CONTRIBUTING.md
   0:0   error  `processSync` finished async. Use `process` instead
                                                                                                                                             null-null
   1:1   error  Delete `⏎`
                                                                                                                                             prettier/prettier
   7:20  error  Delete `·`
                                                                                                                                             prettier/prettier
   8:44  error  Delete `·`
                                                                                                                                             prettier/prettier
   9:68  error  Replace `·This·saves·` with `This·saves`
                                                                                                                                            prettier/prettier
  45:79  error  Replace `⏎I·` with `·I⏎`
                                                                                                                                             prettier/prettier
  51:80  error  Replace `·actions·in·order·to·evaluate·and·enforce·the·project·standards.··` with `⏎actions·in·order·to·evaluate·and·enforce·the·project·standards.·`
                                                                                                                                             prettier/prettier
  52:48  error  Delete `·`
                                                                                                                                             prettier/prettier
  53:30  error  Delete `·`
                                                                                                                                             prettier/prettier
  54:64  error  Replace `·Please·do·your·due⏎diligence.··` with `Please·do·your⏎due·diligence.`
                                                                                                                                             prettier/prettier

✖ 56 problems (55 errors, 1 warning)
  47 errors and 0 warnings potentially fixable with the `--fix` option.

$

In v1.11.0, it runs with the best performance at about 7 seconds for my project. My CI pipeline results are available for review at https://github.com/codejedi365/treemap-js/runs/3407176892?check_suite_focus=true.

JounQin commented 3 years ago

Please try to use "plugin:mdx/recommended" and its settings in overrides firstly .

You're running mdx/remark processor for all files.

See https://github.com/mdx-js/eslint-mdx#notice

JounQin commented 3 years ago 
diff --git a/.eslintrc.js b/.eslintrc.js
index e23d28c..826d5b5 100644
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -5,16 +5,7 @@ module.exports = {
     es2017: true,
     node: true
   },
-  extends: [
-    // JS, MD, MDX
-    "eslint:recommended",
-    "airbnb-base",
-    "plugin:mdx/recommended",
-    "plugin:prettier/recommended"
-  ],
-  settings: {
-    "mdx/code-blocks": true
-  },
+  extends: ["eslint:recommended", "airbnb-base", "plugin:prettier/recommended"],
   ignorePatterns: [
     "**/node_modules",
     "dist/**",
@@ -28,6 +19,13 @@ module.exports = {
     "import/no-extraneous-dependencies": "off"
   },
   overrides: [
+    {
+      files: ["*.md", "*.mdx"],
+      extends: ["plugin:mdx/recommended"],
+      settings: {
+        "mdx/code-blocks": true
+      }
+    },
     {
       files: ["*.json", ".remarkrc"],
       plugins: ["json-format"]
JounQin commented 3 years ago

image

codejedi365 commented 3 years ago

Thank you for the quick reply & fix, that worked significantly. Down to <10 second runtime with the latest version v1.15.

codejedi365 commented 3 years ago

Reopen if necessary...

I spoke too soon. I still have a remark failure TypeError: Cannot read property 'indent' of undefined. What can you tell me about configuring/overriding certain code-block rules and the parser for Typescript code-blocks in markdown?

Failing CI run with TS codeblocks in README.md https://github.com/codejedi365/treemap-js/runs/3432144040?check_suite_focus=true

If I change them to JS code blocks, then it resolves but then doesn't like the Typescript syntax which can be seen here: https://github.com/codejedi365/treemap-js/runs/3432123950?check_suite_focus=true

JounQin commented 3 years ago

@codejedi365

Then it's a bug of eslint-plugin-markdown.

image

JounQin commented 3 years ago

After debugging, message.line is undefined.

{
  "ruleId": null,
  "fatal": true,
  "severity": 2,
  "message":
    "Parsing error: \"parserOptions.project\" has been set for @typescript-eslint/parser.\n" +
    "The file does not match your project config: README.md/0_0.ts.\n" +
    "The file must be included in at least one of the projects provided.",
  "line": undefined,
  "column": undefined
}
JounQin commented 3 years ago

See https://github.com/eslint/eslint-plugin-markdown/pull/191

JounQin commented 3 years ago

@codejedi365 Adding excludedFiles: ["*.md/*.ts", "*.mdx/*.ts"] after https://github.com/codejedi365/treemap-js/blob/main/.eslintrc.js#L32 should resolve your problem.

More details: https://eslint.org/docs/user-guide/configuring/configuration-files#how-do-overrides-work

codejedi365 commented 3 years ago

@JounQin, thank you for continuing to investigate a solution. However,

@codejedi365 Adding excludedFiles: [".md/.ts", ".mdx/.ts"] after https://github.com/codejedi365/treemap-js/blob/main/.eslintrc.js#L32 should resolve your problem.

resolves the undefined error by omitting it to be parsed as Typescript. It then uses the basic JS parser which doesn't throw an exception but rather fails to parse Typescript specific syntax where it is located. Since TS is so closely related to JS, it works on the common syntax. Specifically, the < in fn<type>() does not parse in vanilla JS.

I am pursuing an implementation of a Typescript specific config just for eslint which falls in line with this StackOverflow Post but I haven't gotten it to work yet. I'm not sure how you debugged eslint-plugin-markdown, but the message object post above with the error message hinted to me that its a tsconfig.json include error (of the @typescript-eslint/parser) that is causing the line property to be undefined.

JounQin commented 3 years ago

*.mdx/*.ts means virtual filename, and you can't use some type related ts rules, please read its configuration carefully.

https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/TYPED_LINTING.md

JounQin commented 3 years ago

See also https://github.com/typescript-eslint/typescript-eslint/issues/3174

codejedi365 commented 3 years ago

eslint/eslint-plugin-markdown#191

Glad to see this resolution opened since it would of been helpful to see the error from @typescript-eslint/parser for my debugging of the configuration.

Separately, I looked into these links/recommendations and found a better solution:

@codejedi365 Adding excludedFiles: ["*.md/*.ts", "*.mdx/*.ts"] after https://github.com/codejedi365/treemap-js/blob/main/.eslintrc.js#L32 should resolve your problem.

More details: https://eslint.org/docs/user-guide/configuring/configuration-files#how-do-overrides-work

There were 2 parts to my solution:

  1. excludedFiles was necessary to differentiate the regular TS files from the virtual filenames for markdown-code-blocks.
  2. Create a separate JS & TS configs for the markdown code-block virtual filenames. In the TS config block ignore specific TS rules that actually require tsconfig.json and the local files to exist (see response in typescript-eslint/typescript-eslint#3174). There is only 4 rules that have issues and you can't enable the strict type checking ruleset either.

Overall, this solution enabled codeblocks with TS syntax to be parsed correctly (which the default eslint JS parser cannot do) and apply most of all the other Typescript specific rules to the code contained in markdown-code-blocks. My final configuration looks like:

diff --git a/.eslintrc.js b/.eslintrc.js
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ +6,22 +7,23 @@ module.exports = {
    plugins: ["json-format"]
 },
 {
      files: ["*.ts"],
+     excludedFiles: ["*.m{d,dx}/*.ts"],
      parser: "@typescript-eslint/parser",
      parserOptions: {
        ecmaVersion: 8,
        project: "tsconfig.json"
      },
      plugins: ["@typescript-eslint", "jest", "import"],
      extends: [
        "eslint:recommended",
        "airbnb-typescript/base",
        "plugin:@typescript-eslint/recommended",
        "plugin:@typescript-eslint/recommended-requiring-type-checking",
        "plugin:import/recommended",
        "plugin:import/typescript",
        "plugin:jest/recommended",
        "plugin:jest/style",
        "plugin:prettier/recommended"
      ],
@@ -80,14 +100,46 @@ {
      files: ["*.md", "*.mdx"],
      extends: ["plugin:mdx/recommended"],
      settings: {
        "mdx/code-blocks": true
      }
    },
    {
+     // Markdown JS code-blocks (virtual filepath)
+     files: ["**/*.md/*.{js,jsx}"],
+     rules: {
+       // Invalid rules for embedded code-blocks
+       "import/no-unresolved": "off",
+       "no-undef": "off",
+       "no-unused-expressions": "off",
+       "no-unused-vars": "off",
+       "no-unreachable": "off"
+     }
+   },
+   {
+     // Markdown TS code-blocks (virtual filepath)
+     files: ["**/*.md/*.{ts,tsx}"],
+     parser: "@typescript-eslint/parser",
-     parserOptions: {
-       ecmaVersion: 8,
-       project: "tsconfig.eslint.json"
-     },
+     plugins: ["@typescript-eslint"],
+     extends: [
+       "eslint:recommended",
+       "airbnb-typescript/base",
+       "plugin:@typescript-eslint/recommended",
+       "plugin:prettier/recommended"
+     ],
+     rules: {
+       // Additional embedded code-block invalid rules
+       "import/no-unresolved": "off",
+       "no-undef": "off",
+       "no-unused-expressions": "off",
+       "no-unused-vars": "off",
+       "no-unreachable": "off"
+       "@typescript-eslint/no-unused-vars": "off",
+       // Typescript rules that require a project tsconfig.json which is not possible
+       "@typescript-eslint/dot-notation": "off",
+       "@typescript-eslint/no-implied-eval": "off",
+       "@typescript-eslint/no-throw-literal": "off",
+       "@typescript-eslint/return-await": "off",
+       // Readmes should be extra specific or generic as desired
+       "@typescript-eslint/no-inferrable-types": "off",
+       "@typescript-eslint/ban-types": "warn"
+     }
    }
  ]
};

Proof of resolution: https://github.com/codejedi365/avl-treemap/runs/3448583865?check_suite_focus=true

JounQin commented 3 years ago

Yes, you're separating code blocks and normal physical .ts files correctly now.