Issue in TYPESCRIPT-ES when using mega-lint-runner

[MRDGH2821]

Describe the bug

When using mega-lint-runner, TYPESCRIPT_ES always fails to lint, because it is not able to access /tsconfig.json

Where as in Github Actions, there is no error for the same.

Expected behavior TYPESCRIPT_ES should have thrown error or success flag on both CI & local, but it is not happenning.

Screenshots Here is the log file instead

ERROR-TYPESCRIPT_ES.log

Additional context My repository where it is enabled - https://github.com/MRDGH2821/Perpetual-Mechanical-Array-Bot/tree/rewrite-detritus-bug-hunt Branch - rewrite-detritus-bug-hunt The configurations are at root of the directory.

.eslintrc.json -

{
  "extends": ["eslint-config-airbnb-base", "eslint-config-airbnb-typescript/base"],
  "parserOptions": {
    "project": ["tsconfig.json", "tsconfig.eslint.json"]
  },
  "rules": {
    "no-console": "off"
  }
}

tsconfig.json -

{
  "compilerOptions": {
    "allowJs": false,
    "baseUrl": "./src",
    "declaration": true,
    "esModuleInterop": true,
    "experimentalDecorators": true,
    "importHelpers": true,
    "inlineSources": true,
    "lib": ["ES2019", "DOM", "ESNext"],
    "module": "CommonJS",
    "moduleResolution": "node",
    "noImplicitAny": true,
    "noImplicitReturns": true,
    "outDir": "out",
    "sourceMap": true,
    "strict": true,
    "strictNullChecks": true,
    "target": "ESNext",
    "resolveJsonModule": true,
    "watch": false
  },
  "ts-node": {
    "require": ["tsconfig-paths/register"],
    "swc": true
  },
  "tsc-alias": {
    "resolveFullPaths": true
  },
  "include": ["src/**/*.ts"],
  "exclude": ["node_modules", ".vscode", "./old-src", "./out/*"]
}

[Kurt-von-Laven]

I am moving this discussion from #2578, where this is the third of three issues, to this thread. If you've already read that thread, this will be redundant, but I will summarize it for those who haven't.

The root cause of the issue I reported on the other thread is related to the way we install our Node.js dependencies, namely to /node-deps. ESLint is leaning on TypeScript to find the @tsconfig/node18-strictest-esm package that gets installed to /node-deps via a pre-command, but TypeScript neither honors NODE_PATH nor has its own configuration option to search a directory other than node_modules when resolving packages in extends of tsconfig.json. I was able to confirm this theory in a minimal reproduction with a small Dockerfile, and the following workaround works for us:

POST_COMMANDS:
  - command: rm -r node_modules
    cwd: workspace
TYPESCRIPT_ES_PRE_COMMANDS:
  - command: npm install @tsconfig/node18-strictest-esm@1.0.1
  - command: cp --recursive /node-deps/node_modules/ node_modules
    cwd: workspace

One thing that would potentially make this all better would be supporting arrays in the *_CLI_EXECUTABLE config option, which I suspect might be a one-line change. That would allow users to configure MegaLinter to run ESLint via Yarn, allowing TypeScript to find ESLint installed via Yarn PnP. It's a little silly in the sense that it defeats the purpose of having ESLint and friends in the Docker image at all. On the other hand, it's generally a good idea to add those directly to your project as dev dependencies anyways to keep your IDE in sync with MegaLinter. Reopening this issue now that it's actionable again. Open to suggestions on better ways of dealing with this.

[Kurt-von-Laven]

Copied from #2578:

@Kurt-von-Laven I'm trying to fix that in https://github.com/oxsecurity/megalinter/pull/2601 ... the idea is the following:

• check if PRE_COMMANDS contain "npm" or "yarn"
• if yes:
  • copy /node_deps/node_modules into |workspace+"node_modules"
  • Add workspace+"node_modules" in PATH
  • run the PRE_COMMANDS

Do you think it could work ? ^^

@Kurt-von-Laven i tried to reproduce your issue and it seems to work... please can you confirm with the newest beta version ? :)

https://github.com/oxsecurity/megalinter/actions/runs/4846602288/jobs/8636240640

@nvuillam, #2601 does not fix the issue and breaks the workaround.

I tried adapting the workaround I described above by specifying cwd: /node-deps for the npm install command, but I encountered the following error:

multiprocessing.pool.RemoteTraceback:                                                                                                                                                        """                                                                                                                                                                                          Traceback (most recent call last):
  File "/usr/local/lib/python3.11/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
                           ^^^^^^^^^^^^
  File "/megalinter/MegaLinter.py", line 39, in run_linters
    linter.run()
  File "/megalinter/Linter.py", line 711, in run
    pre_post_factory.run_linter_pre_commands(self.master, self)
  File "/megalinter/pre_post_factory.py", line 24, in run_linter_pre_commands
    return run_commands(
              ^^^^^^^^^^^
  File "/megalinter/pre_post_factory.py", line 52, in run_commands
    pre_command_result = run_command(command_info, log_key, mega_linter, linter)
                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/megalinter/pre_post_factory.py", line 89, in run_command
    raise Exception(
Exception: [Pre][TYPESCRIPT_ES]: User command failed, stop running MegaLinter
npm ERR! Tracker "idealTree" already exists
npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2023-05-01T06_35_02_059Z-debug-0.log                                                                                                                                                                                                                                                                                                                        

Leaving the workaround as-is results in the same crash on npm install. That doesn't seem like a sensible approach though since the workaround intends to install the package to /node-deps, and #2601 removed the logic that changes directories to /node-deps for npm install commands. I still think that makes the developer experience less friendly (c.f., https://github.com/oxsecurity/megalinter/issues/2578#issuecomment-1525068133). Removing the workaround gets us back to lots of: 0:0 error Parsing error: File '@tsconfig/node18-strictest-esm/tsconfig.json' not found. That error comes from the tsconfig we extend not being present in /tmp/lint/node_modules. Copying /node-deps/node_modules to /tmp/lint/node_modules before running pre-commands doesn't address this since @tsconfig/node18-strictest-esm correctly isn't in the MegaLinter Docker image. I don't think we want to be automatically copying /node-deps/node_modules to /tmp/lint/node_modules when a pre-command contains "npm" or "yarn". Our users are not likely to expect this surprising behavior, we don't have a use case to motivate it, and it seems likely to create confusion or even breakage in Yarn projects that don't use node_modules. From https://github.com/oxsecurity/megalinter/issues/2578#issuecomment-1525068133:

In addition, if they didn't think to clean node_modules with a post-command, Yarn PnP (or pnpm) projects would end up with two copies of all of their the dependencies, one of which (the one in node_modules) is probably owned by root and isn't used by Yarn/pnpm. This can create confusion, particularly since the versions most likely wouldn't stay in sync as they have separate lock files managed by different tools.

[nvuillam]

@Kurt-von-Laven thanks for the feedback

The goal is to be able to perform npm install, yarn install, npm install xxx-dep or yarn add xxx-dep in PRE-COMMANDS without any workaround, don't worry i don't plan to release a new ML version before that :)

There was a bug, and i also added dir_exists_ok =True in another PR, it might work better

[Kurt-von-Laven]

Pre/post-commands containing npm install [<pacakge>] and yarn add [<pacakge>] already worked before to my knowledge, just not without the workaround of copying the package to /tmp/lint/node_modules when the missing package is referenced by the extends field of a tsconfig.json. I haven't ever tried running yarn add from a pre-command, but also I can't think of a plausible reason to. I still don't know a way to make that edge case smoother without making the situation worse overall beyond just permitting arrays in *_CLI_EXECUTABLE. Forcing the copy to overwrite /tmp/lint/node_modules seems likely to create even more disruption for users. What happens if they are using npm (or Yarn v1 or Yarn v2+ configured with nodeLinker: node-modules) but have a different version of, say, typescript?

[nvuillam]

Everything changes since all commands are called from workspace and not from docker image root, there are more advantages than inconveniences to call linters the same way they would be locally.

I'm currently working on your idea to allow to override cli_executable

[Kurt-von-Laven]

Changing the default working directory of pre-commands from / to /tmp/lint seems good. I still think overwriting /tmp/lint/node_modules is quite disruptive though.

[nvuillam]

@Kurt-von-Laven I'm trying again something else... :)

Would it be acceptable to say that if there are additional npm dependencies:

• they must be in package.json
• npm install must be called in PRE_COMMANDS
• linter_CLI_EXECUTABLE must be overriden (this is now possible in beta, and it was not just 2 lines :D) with ["npm","run","somelinter"] or something similar ?

[Kurt-von-Laven]

That sounds promising!

[nvuillam]

@Kurt-von-Laven I think i have found the solution, published in the newest beta ! :)

--> https://github.com/nvuillam/CSpell-Bug/tree/nico (additional cspell dict is well added, without copying node_modules or messing with PATH)

Job with GHA: https://github.com/nvuillam/CSpell-Bug/actions/runs/4854021395/jobs/8650882718?pr=1

Job with mega-linter-runner: https://github.com/nvuillam/CSpell-Bug/actions/runs/4854021390/jobs/8650882471?pr=1

Please can you try by using cwd: workspace in your PRE_COMMANDS with npm install ?

[Kurt-von-Laven]

Running npm install [<package>] in the workspace breaks Yarn projects that don't use node_modules:

On the one hand, running npm install @tsconfig/node18-strictest-esm@1.0.1 in the workspace would install the module directly in the place TypeScript will ultimately look for it. On the other, it will also install everything in your package.json into the node_modules directory in your workspace that you didn't think you were using and create package-lock.json. I haven't checked pnpm, but neither npm nor Yarn offer a means by which to install only a single package to node_modules in their CLIs. Installing all packages could be a considerable slowdown for projects with large dependency trees, particularly since they may not think to cache npm dependencies in CI if they aren't using npm directly. In addition, if they didn't think to clean node_modules with a post-command, Yarn PnP (or pnpm) projects would end up with two copies of all of their the dependencies, one of which (the one in node_modules) is probably owned by root and isn't used by Yarn/pnpm. This can create confusion, particularly since the versions most likely wouldn't stay in sync as they have separate lock files managed by different tools. At the end of the day, I think this issue demonstrates the subtle pitfalls of running Node.js modules via a different package manager than the one a project is using. ~ https://github.com/oxsecurity/megalinter/issues/2578#issuecomment-1525068133

[Kurt-von-Laven]

I hit the same npm ERR! Tracker "idealTree" already exists error from https://github.com/oxsecurity/megalinter/issues/1572#issuecomment-1529427943 when I put the following in .mega-linter.yaml:

TYPESCRIPT_ES_PRE_COMMANDS:
  - command: npm install @tsconfig/node18-strictest-esm@1.0.1
    continue_if_failed: false
    cwd: /node-deps

When I use cwd: workspace instead, I get lots of: 0:0 error Parsing error: File '@tsconfig/node18-strictest-esm/tsconfig.json' not found. This surprised me, and I also found it odd that node_modules/@tsconfig existed in the workspace but was empty. Even if we were to troubleshoot that though, running npm install with cwd: workspace will always create many problems in a Yarn Plug'n'Play project. Beyond the aforementioned issues, running yarn install after npm install causes Yarn PnP to migrate itself back to the incompatible v1 lock file format and configure itself to use node_modules.

Copying node_modules into the workspace means that it will be owned by root if you aren't using rootless Docker, breaking all subsequent npm commands. We shouldn't be doing that even behind a flag. The copy takes place before the user's pre-commands run, so it doesn't help TypeScript find an extended tsconfig. I'm not aware of another use case that copying node_modules addresses. Copying node_modules after a pre-command containing npm install runs would be even worse since at that point the linters are already running, resulting in unpredictable behavior.

[nvuillam]

@Kurt-von-Laven did you pull the latest beta ? There is no more copy of node_modules from MegaLinter core cwd can be only root or workspace (root by default), if you want to go in another folder you need to cd directly in the command

[Kurt-von-Laven]

Yeah, sorry, I think I was under the impression when I wrote that that the node_modules copying was still taking place in certain circumstances, but that was probably based on my outdated code review. I am not experiencing the node_modules copying now on the same beta image in any case, so that's exciting! Ohhh... thanks for the tip about cwd. ESLint ran this time with my original workaround where I copied /node-deps/node_modules/@tsconfig to /tmp/lint/node_modules. I hit hundreds of @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access, and @typescript-eslint/restrict-template-expressions errors though, which makes me think typescript-eslint lost the ability to load dependencies properly since Thursday April 27th when it was last working. This is kind of a stab in the dark, but what are we doing with NODE_PATH and PATH at this point?

[nvuillam]

@Kurt-von-Laven with newest beta, NODE_PATH and PATH remain untouched, NODE_PATH is /node-deps and PATH still has /node-deps/node_modules

[nvuillam]

@Kurt-von-Laven I'm trying to add a test case, do you have suggestions about how it should be built ?

I'm trying that, probably it needs an additional package.json, maybe a 'yarn' to install dependencies as pre-command ?

[Kurt-von-Laven]

That is a great idea! In an empty directory, one can run yarn set version stable to use the latest stable version of Yarn instead of Yarn v1 and create a minimal package.json. Yarn v1 is already end-of-life, so I don't think we need to worry about a separate test case for it. yarn add --dev @tsconfig/node18-strictest-esm eslint typescript@4.9.5 @typescript-eslint/parser @typescript-eslint/eslinter-plugin will add dev dependencies to the package.json and install it to .yarn/cache. One could copy .pnp.cjs, .yarn, .yarnrc.yml, package.json, and yarn.lock to a test fixture directory within MegaLinter for use in a test. The npm install pre-command is only needed if one isn't running ESLint via Yarn. Running ESLint via Yarn necessitates adding the required ESLint-related dependencies via Yarn. See .eslintrc.yaml and tsconfig.json from https://github.com/ScribeMD/docker-cache/ for help configuring things properly if needed.

[Kurt-von-Laven]

One issue I expect this test will expose is that TYPESCRIPT_ES_CLI_EXECUTABLE: [yarn, run, eslint] is only sufficient to run Yarn v1, so hopefully the test will catch that and fail. jwkvam/bowtie#222 has some discussion of yarn not working properly when run via subprocess.run rather than via the command line. Personally, I encountered Yarn v1 being used instead of Yarn v2+ since the former is installed globally, but the latter is only installed locally to the project and hence isn't on the PATH. GPT-3.5 suggested TYPESCRIPT_ES_CLI_EXECUTABLE: [node, .yarn/releases/yarn-*.cjs, run, eslint] instead, which seems right to me and works when I test it outside of MegaLinter directly on the command line.

[Kurt-von-Laven]

TYPESCRIPT_ES_CLI_EXECUTABLE: [node, .yarn/releases/yarn-3.6.3.cjs, run, eslint] is working well for us running ESLint in list_of_files mode. This resolves the original Parsing error: Cannot read file '/tsconfig.json' issue for Yarn users. If anyone is still encountering this error, we can reopen this issue or open a new issue as appropriate. If anyone has suggestions regarding a better solution, I am all ears.