Fix tsc setup to report project-relative paths in error messages

[bajtos]

Right now, when typescript compiler encounters an error, it reports file paths relative to individual packages (e.g. src/index.ts) instead of paths relative to monorepo root (e.g. `packages/core/index.ts).

This makes fixing error tedious, because one has to parse text around the error message to find out which package the problematic file belongs to.

Another issue is that VS Code's problem matcher is not able to associate the error message with the specific file (and location in that file).

We need to fix the output of npm test to report paths relative to monorepo root, and preferably also fix the problem matcher in VS Code to deal with the prefix added to error messages by lerna when running in parallel mode.

Acceptance criteria

• When I run npm test and the compiler reports compilation errors, these errors contain file names relative to monorepo root (not to individual packages). See Verify TypeScript setup in DEVELOPING.

• When I run "Build, test and lint" task from VS Code and the compiler reports compilation errors, these errors are shown in VSCode's PROBLEMS window. When I click on an error, VSCode jumps to the exact line causing the error. See Verify TypeScript setup >> Compilation Errors in VSCODE.

[bajtos]

Cross-posting my comment from #977.

FWIW: this used to work before, I suspect #682 broke it.

The trick is that we need to run tsc from the root monorepo directory.

https://github.com/strongloop/loopback-next/pull/682/files#diff-272a4137cf0e755785f43213ae0bae7fL14

Under the hood, we change the current directory to loopback-next root
and call tsc with project configuration from the original package's
tsconfig.build.json. This way tsc error messages contain a path
that's relative to loopback-next project root.

https://github.com/strongloop/loopback-next/pull/682/files#diff-272a4137cf0e755785f43213ae0bae7fL60

'-p',
// It's important to keep this path relative to rootDir
path.join(packageDir, 'tsconfig.build.json'),

I have some ideas how to fix this (basically introduce a new lb-tsc argument like -r ../.. that will control the working directory of tsc), but I prefer to leave such changes out of this documentation-only pull request (#977).

[jannyHou]

Take the following error report as an example:

@loopback/[email protected] build /Users/jannyhou/lb-next/v3/new-decorator/loopback-next/packages/openapi-v3
> lb-tsc es2017
 src/controller-spec.ts(17,9): error TS2305: Module '"/Users/jannyhou/lb-next/v3/new-decorator/loopback-next/packages/openapi-v3/src/index"' has no exported member 'jsonToSchemaObject'.

The line above error report specifies the package path, we need some discussion on the current error report flavor and the one @bajtos suggests in this issue.

BTW, please note the latest vscode shows files with compile error.

[dhmlau]

From the discussion in today's estimation meeting, @raymondfeng thinks this is working as expected. The package name can be found 2 lines above the error. See @jannyHou 's example above

What's needed to be discussed is whether it should be a bug. @raymondfeng and @bajtos seem to have different views.

[raymondfeng]

To be accurate, I don't think there is a right/wrong answer here. It's about preference. The current build runs tsc per package and reports compilation errors using relative path to the package root (not the monorepo root). I don't anticipate many compilation errors and IDEs such as VS code is able to catch most of them now. The project root is printed out by lerna as @jannyHou's example output shows.

While I feel the current behavior is fine, I'm open to see @bajtos's idea. But I don't think we have to do it for MVP.

[bajtos]

I don't anticipate many compilation errors and IDEs such as VS code is able to catch most of them now.

I am afraid this is not true. IDE can catch errors only in the files you are editing, it does not report errors in other files. Project-wide refactorings like #1050 are difficult in the current setup. When a widely-used entity (e.g. ctx.get) changes, we need to run the compiler to find all places that need to be changed too, VS Code is not able to find them on its own.

To be accurate, I don't think there is a right/wrong answer here. It's about preference. The current build runs tsc per package and reports compilation errors using relative path to the package root (not the monorepo root).

I understand some people may prefer to see package-relative paths in tsc output, in which case I agree there is no right/wrong answer.

However, I still want a working PROBLEMS window in VS Code. If there is a way how to get that functionality using package-relative paths then I am fine with keeping package-relative paths.

[bajtos]

BTW, please note the latest vscode shows files with compile error.

Have you tried to click on the line in PROBLEMS window? Does it open the file for you, or does it print an error about an unknown file?

This is kind of errors I am getting (my monorepo root is file:///Users/bajtos/src/loopback/next):

Unable to open 'inject.ts': File not found (file:///Users/bajtos/src/loopback/context/src/inject.ts).
Unable to open 'http-handler.ts': File not found (file:///Users/bajtos/src/loopback/next/@loopback/authentication: ../rest/src/http-handler.ts).

I think the first problem happens when lerna groups multiple error message from the same package and prints the package name prefix on the first line only. The second problem comes from a line that includes package name prefix from lerna.

[bajtos]

I am proposing to modify lb-tsc to detect Lerna presence and run tsc from the monorepo root directory when Lerna is used. To do so, I need Lerna to provide an environment variable pointing to monorepo root - see lerna/lerna#1343.

In my proposal, people running lb-tsc in a single-package repository will keep seeing paths relative to their package root. Same behavior will apply when running npm run build from a package directory inside monorepo.

Only when invoking lb-tsc via Lerna, paths will be printed relative to monorepo root.

@raymondfeng thoughts/objections?

[raymondfeng]

@bajtos I created #1189 to allow env vars to be passed to commands.

To verify LERNA_ROOT_PATH is passed to the downstream commands:

node ./bin/run-lerna.js exec "node -e 'console.log(process.env.LERNA_ROOT_PATH);'"

[raymondfeng]

@bajtos I cannot find an elegant way to do what you want. Here is what I have tried to implement with LERNA_ROOT_PATH and lb-tsc:

lb-tsc -p <LERNA_ROOT_PATH>/tsconfig.json -outDir dist

Please note the location of tsconfig.json controls the relative path.

There is a conflict of interest:

1. In LERNA_ROOT_PATH>/tsconfig.json, we have "includes": "packages" to contain TS files from all packages
2. For a given package, we only want to have packages/<pkg-name> to be included.
   The only option I see is to generate (temporary) package specific tsconfig.json at root level. It is pretty ugly.

[bajtos]

I cannot find an elegant way to do what you want.

The idea is to execute tsc in monorepo root directory and point it to tsconfig.json living in the per-package directory.

loopback-next$ tsc packages/core/tsconfig.json --target es2017

In my experience so far, any configuration provided in tsconfig.json is relative to the directory where tsconfig.json is located.

Below is the hard-coded solution that I am using as a workaround right now, it seems to work flawlessly to me.

index a5c8ec7e..ed7f5f12 100755
--- a/packages/build/bin/compile-package.js
+++ b/packages/build/bin/compile-package.js
@@ -98,10 +98,12 @@ function run(argv, options) {
     }
   }

+ // TODO: replace with LERNA_ROOT_PATH
+  const cwd = path.resolve(path.dirname(tsConfigFile), '../..');
+
   const args = [];

   if (tsConfigFile) {
-    args.push('-p', tsConfigFile);
+    args.push('-p', path.relative(cwd, tsConfigFile));
   }

   if (outDir) {
@@ -114,7 +116,7 @@ function run(argv, options) {

   args.push(...compilerOpts);

-  return utils.runCLI('typescript/lib/tsc', args, options);
+  return utils.runCLI('typescript/lib/tsc', args, {cwd, ...options});
 }

In order to allow VSCode's problem matcher to parse compiler errors, I am disabling parallel mode:

+++ b/package.json
@@ -35,7 +35,7 @@
     "prettier:fix": "npm run prettier:cli -- --write",
     "clean": "lerna run clean --loglevel=silent --parallel",
     "clean:lerna": "lerna clean --yes --parallel --loglevel=silent",
-    "build": "lerna run build --parallel --loglevel=silent",
+    "build": "lerna run build --loglevel=silent",
     "build:full": "npm run clean:lerna && npm run bootstrap && npm test",
     "pretest": "npm run clean && npm run build",
     "test": "node packages/build/bin/run-nyc npm run mocha --scripts-prepend-node-path",

With these two changes in place, both acceptance criteria are met, at the cost of a much slower build step.

[bajtos]

There is a conflict of interest:

1. In LERNA_ROOT_PATH>/tsconfig.json, we have "includes": "packages" to contain TS files from all packages

The monorepo-root level tsconfig.json file is intended only for TypeScriptServer run by VisualStudio Code, not for the actual build.

2. For a given package, we only want to have packages/<pkg-name> to be included.
   The only option I see is to generate (temporary) package specific tsconfig.json at root level. It is pretty ugly.

As I commented above, you can tell tsc to compile a project living in a different working directory. This way the compiler include only package files, but compiler errors prints paths relative to the current working directory (the monorepo root).

[raymondfeng]

In order to allow VSCode's problem matcher to parse compiler errors, I am disabling parallel mode:

Do you run the script from inside VSCode? Is there a possibility to set an env var? If so, we can improve run-lerna to set --parallel conditionally.

[raymondfeng]

I have implemented it in #1189.

The run-lerna script adds the following logic:

1. Set LERNA_ROOT_PATH env var
2. Detect if the script is run within VSCode and disable --parallel if so